Backend table schema#1742
Merged
Merged
Conversation
- Implemented mapping function to convert schema change entities to response DTOs. - Created a guard to manage ownership validation for schema changes. - Added validation utility for proposed DDL statements, enforcing rules and structure. - Introduced migration to create the `table_schema_change` entity with necessary fields and constraints. - Developed end-to-end tests for MySQL and PostgreSQL to validate schema change generation, approval, rollback, and rejection scenarios. - Updated pnpm lockfile to include new dependencies for SQL parsing.
…racle - Implemented MSSQL E2E tests for creating, approving, rolling back tables, columns, and indexes. - Added tests for handling invalid SQL, destructive changes, and user-modified SQL in MSSQL. - Created Oracle E2E tests for creating tables, adding columns, and altering column types while preserving data. - Ensured proper cleanup of test tables and cache after tests.
- Implemented MongoDB schema change operations including createCollection, dropCollection, setValidator, createIndex, and dropIndex. - Enhanced the GenerateSchemaChangeUseCase and RollbackSchemaChangeUseCase to handle MongoDB operations. - Introduced validation for proposed MongoDB operations to ensure structural safety and prevent forbidden operations. - Updated the assertDialectSupported utility to include MongoDB as a supported dialect. - Created a new utility for MongoDB schema operations and validation. - Added migration to extend the schema change type enum to include MongoDB types. - Developed comprehensive end-to-end tests for MongoDB schema changes, covering various scenarios including creation, rollback, and validation.
…ement related tests - Extend the SchemaChangeTypeEnum to include DynamoDB operations: CREATE_TABLE, DROP_TABLE, UPDATE_TABLE, and UPDATE_TTL. - Create a migration to update the database schema for the new DynamoDB change types. - Implement end-to-end tests for DynamoDB schema changes, including creating, dropping, updating tables, and managing TTL. - Ensure rollback functionality is tested for all operations, validating that changes can be reversed as expected. - Add validation for user-modified SQL operations to ensure consistency and correctness.
- Implemented Elasticsearch schema change operations including createIndex, deleteIndex, and updateMapping. - Updated use cases for approving, applying, and rolling back schema changes to handle Elasticsearch. - Added validation for proposed Elasticsearch operations. - Created utility functions for executing Elasticsearch schema operations. - Enhanced the dialect support to include Elasticsearch in the system. - Added migration to update the schema change type enum to include Elasticsearch types. - Developed end-to-end tests for Elasticsearch schema changes, ensuring correct behavior for create, delete, and update operations. - Implemented error handling for unsupported dialects, specifically for Redis.
…dle multi-table batch changes - Updated proposal handling to support multiple proposals in a single request across various database tests. - Adjusted test assertions to correctly parse and validate changes from the response, ensuring compatibility with the new proposal structure. - Enhanced error handling and rollback mechanisms for batch operations to maintain data integrity during failures. - Added new tests for multi-table batch generation and approval, ensuring that changes are applied in the correct order and that rollback functionality works as expected.
…SchemaChangeEntity
📝 WalkthroughWalkthroughA comprehensive table schema change management system is introduced, enabling AI-driven database schema modifications across SQL, MongoDB, DynamoDB, Elasticsearch, ClickHouse, and Cassandra. The feature includes generation, approval, rejection, rollback workflows with database state persistence, REST API endpoints, and end-to-end test coverage across all supported database backends. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as TableSchema Controller
participant UseCase as Generate/Approve UseCase
participant AIService as AI Core Service
participant Validator as Validator/Executor
participant DB as Database
participant Target as Target DB
Client->>Controller: POST /generate (prompt)
Controller->>UseCase: execute(GenerateSchemaChangeDs)
UseCase->>AIService: runSchemaChangeAiLoop()
loop AI Iterations
AIService->>Target: Call inspection tool (getTableStructure)
Target-->>AIService: Table metadata
AIService->>AIService: Generate proposal with tool call
end
AIService-->>UseCase: SchemaChangeAiLoopResult
UseCase->>Validator: validateProposedDdl/Op (for each proposal)
Validator-->>UseCase: Validated or error
UseCase->>DB: createPendingBatch(proposals)
DB-->>UseCase: SchemaChangeBatchResponseDto
UseCase-->>Controller: Response
Controller-->>Client: 200 + batch with PENDING changes
Client->>Controller: POST /approve (changeId, confirmedDestructive)
Controller->>UseCase: execute(ApproveSchemaChangeDs)
UseCase->>DB: findByIdWithRelations(changeId)
DB-->>UseCase: TableSchemaChangeEntity
alt Change not reversible and not confirmed
UseCase-->>Client: 400 destructive_confirmation_required
else Valid
UseCase->>Validator: validateProposedDdl (if userModifiedSql)
UseCase->>DB: transitionStatusIfMatches (PENDING→APPLYING)
UseCase->>Target: executeSchemaChange(forwardSql)
Target-->>UseCase: Success or error
alt Execution failed
UseCase->>Target: executeSchemaChange(rollbackSql) [optional auto-rollback]
Target-->>UseCase: Rollback result
UseCase->>DB: updateStatus(FAILED, with error/rollback flags)
else Execution succeeded
UseCase->>DB: updateStatus(APPLIED, appliedAt)
end
UseCase->>DB: findByIdWithRelations (re-read)
UseCase-->>Controller: SchemaChangeResponseDto
Controller-->>Client: 200 + updated change
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an AI-driven “table schema change” backend feature: generate schema-change proposals, store/audit them, and allow approve/reject/rollback for single changes or batches across multiple database dialects.
Changes:
- Introduces
TableSchemaChangepersistence (entity + migrations) including batching (batchId,orderInBatch) and new change-type enums (Mongo/DynamoDB/Elasticsearch). - Adds Table Schema module/controller + use-cases for generate, approve/apply, reject, list, get, and rollback (single + batch).
- Adds dialect-specific execution/validation helpers (SQL via
node-sql-parser, plus Mongo/DynamoDB/Elasticsearch structured ops, ClickHouse/Cassandra executors) and new AVA e2e coverage.
Reviewed changes
Copilot reviewed 70 out of 71 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks new dependency versions (incl. node-sql-parser). |
| backend/package.json | Adds node-sql-parser dependency. |
| backend/test/ava-tests/non-saas-tests/non-saas-table-schema-redis-e2e.test.ts | E2E: unsupported dialect (redis) returns 400 without AI call. |
| backend/test/ava-tests/non-saas-tests/non-saas-table-schema-oracle-e2e.test.ts | E2E: Oracle generate/approve/rollback scenarios. |
| backend/test/ava-tests/non-saas-tests/non-saas-table-schema-ibmdb2-e2e.test.ts | E2E: DB2 generate/approve/rollback scenarios. |
| backend/test/ava-tests/non-saas-tests/non-saas-table-schema-elasticsearch-e2e.test.ts | E2E: Elasticsearch op flows (create/delete/updateMapping, destructive confirmation, invalid ops). |
| backend/test/ava-tests/non-saas-tests/non-saas-table-schema-clickhouse-e2e.test.ts | E2E: ClickHouse DDL flows + destructive confirmation + userModifiedSql validation. |
| backend/test/ava-tests/non-saas-tests/non-saas-table-schema-cassandra-e2e.test.ts | E2E: Cassandra CQL flows + destructive confirmation + userModifiedSql validation. |
| backend/src/migrations/1776857063726-AddTableSchemaChangeEntity.ts | Creates table_schema_change table + base enums/indexes. |
| backend/src/migrations/1776953988756-AddMongoSchemaChangeTypes.ts | Extends enum with Mongo change types. |
| backend/src/migrations/1777039182865-AddDynamoDbSchemaChangeTypes.ts | Extends enum with DynamoDB change types. |
| backend/src/migrations/1777272567941-AddElasticsearchSchemaChangeTypes.ts | Extends enum with Elasticsearch change types. |
| backend/src/migrations/1777295364595-AddBatchColumnsToTableSchemaChange.ts | Adds batchId / orderInBatch + batch ordering index. |
| backend/src/entities/table-schema/utils/assert-dialect-supported.ts | Dialect gating + parser-dialect mapping helpers. |
| backend/src/entities/table-schema/utils/validate-proposed-ddl.ts | SQL DDL safety validation (regex gates + optional AST checks). |
| backend/src/entities/table-schema/utils/execute-schema-change.ts | Dispatches execution by dialect/type (SQL/Mongo/DynamoDB/Elasticsearch/ClickHouse/Cassandra). |
| backend/src/entities/table-schema/utils/mongo-schema-op.ts | Validates + executes structured Mongo schema operations. |
| backend/src/entities/table-schema/utils/elasticsearch-schema-op.ts | Validates + executes structured Elasticsearch schema operations. |
| backend/src/entities/table-schema/utils/clickhouse-ddl.ts | Executes ClickHouse DDL via HTTP client. |
| backend/src/entities/table-schema/utils/cassandra-ddl.ts | Executes Cassandra CQL via driver. |
| backend/src/entities/table-schema/utils/map-schema-change-to-response-dto.ts | Maps entity → response DTO. |
| backend/src/entities/table-schema/utils/schema-change-ownership.guard.ts | Guard: authorize access to a change by connection permission. |
| backend/src/entities/table-schema/utils/schema-change-batch-ownership.guard.ts | Guard: authorize access to a batch by connection permission. |
| backend/src/entities/table-schema/table-schema-change-enums.ts | Defines schema change statuses/types + helpers. |
| backend/src/entities/table-schema/table-schema-change.entity.ts | TypeORM entity for schema-change records (incl. batching). |
| backend/src/entities/table-schema/repository/table-schema-change.repository.interface.ts | Repository contract for schema-change persistence/queries. |
| backend/src/entities/table-schema/repository/custom-table-schema-change-repository-extension.ts | TypeORM repository extension implementation. |
| backend/src/entities/table-schema/application/data-transfer-objects/schema-change-response.dto.ts | API DTO for single schema-change response. |
| backend/src/entities/table-schema/application/data-transfer-objects/schema-change-list-response.dto.ts | API DTO for paginated history listing. |
| backend/src/entities/table-schema/application/data-transfer-objects/schema-change-batch-response.dto.ts | API DTO for batch envelope. |
| backend/src/entities/table-schema/application/data-transfer-objects/generate-schema-change.dto.ts | API DTO for generate request. |
| backend/src/entities/table-schema/application/data-transfer-objects/approve-schema-change.dto.ts | API DTO for approve/apply request. |
| backend/src/entities/table-schema/application/data-transfer-objects/approve-batch-schema-change.dto.ts | API DTO for batch approve request. |
| backend/src/entities/table-schema/application/data-structures/*.ts | Use-case input data structures for table-schema endpoints. |
| backend/src/entities/table-schema/ai/schema-change-tools.ts | AI tool definitions for SQL/Mongo/DynamoDB/Elasticsearch schema changes. |
| backend/src/entities/table-schema/ai/schema-change-prompts.ts | Prompt builders per dialect (SQL vs NoSQL structured ops). |
| backend/src/entities/table-schema/ai/run-schema-change-ai-loop.ts | AI loop runner with tool-call enforcement and inspection calls. |
| backend/src/entities/table-schema/use-cases/table-schema-use-cases.interface.ts | Use-case interfaces for DI. |
| backend/src/entities/table-schema/use-cases/generate-schema-change.use-case.ts | Generates proposals via AI loop; persists as batch. |
| backend/src/entities/table-schema/use-cases/approve-and-apply-schema-change.use-case.ts | Applies a single schema change (with optional userModifiedSql). |
| backend/src/entities/table-schema/use-cases/approve-batch-schema-changes.use-case.ts | Applies a batch sequentially with cascade rollback on failure. |
| backend/src/entities/table-schema/use-cases/reject-schema-change.use-case.ts | Rejects a single pending change. |
| backend/src/entities/table-schema/use-cases/reject-batch-schema-changes.use-case.ts | Rejects all rejectable changes in a batch. |
| backend/src/entities/table-schema/use-cases/rollback-schema-change.use-case.ts | Rolls back a single applied change + records rollback audit row. |
| backend/src/entities/table-schema/use-cases/rollback-batch-schema-changes.use-case.ts | Rolls back applied items in a batch (reverse order) + audit rows. |
| backend/src/entities/table-schema/use-cases/list-schema-changes.use-case.ts | Lists schema-change history with pagination. |
| backend/src/entities/table-schema/use-cases/get-schema-change.use-case.ts | Fetches a single schema change by id. |
| backend/src/entities/table-schema/use-cases/get-batch-schema-changes.use-case.ts | Fetches all changes in a batch by batchId. |
| backend/src/entities/table-schema/table-schema.controller.ts | HTTP API endpoints for schema-change operations. |
| backend/src/entities/table-schema/table-schema.module.ts | Nest module wiring for controller/use-cases/guards. |
| backend/src/decorators/slug-uuid.decorator.ts | Extends SlugUuid decorator to accept changeId/batchId. |
| backend/src/common/data-injection.tokens.ts | Adds new UseCaseType tokens for schema-change use-cases. |
| backend/src/common/application/global-database-context.ts | Registers schema-change repository extension in global DB context. |
| backend/src/common/application/global-database-context.interface.ts | Exposes schema-change repository on global DB context interface. |
| backend/src/app.module.ts | Wires TableSchemaModule into the application module list. |
| backend/src/ai-core/tools/prompts.ts | Adds readable names for new DB types (ClickHouse/DynamoDB/Cassandra/Elasticsearch). |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3
to
+8
| import sqlParser from 'node-sql-parser'; | ||
|
|
||
| const { Parser } = sqlParser; | ||
|
|
||
| import { isMongoSchemaChangeType, SchemaChangeTypeEnum } from '../table-schema-change-enums.js'; | ||
| import { connectionTypeToParserDialect } from './assert-dialect-supported.js'; |
Comment on lines
+4
to
+18
| const SUPPORTED_DIALECTS: ReadonlySet<ConnectionTypesEnum> = new Set([ | ||
| ConnectionTypesEnum.postgres, | ||
| ConnectionTypesEnum.mysql, | ||
| ConnectionTypesEnum.mysql2, | ||
| ConnectionTypesEnum.mssql, | ||
| ConnectionTypesEnum.oracledb, | ||
| ConnectionTypesEnum.ibmdb2, | ||
| ConnectionTypesEnum.mongodb, | ||
| ConnectionTypesEnum.clickhouse, | ||
| ConnectionTypesEnum.agent_clickhouse, | ||
| ConnectionTypesEnum.dynamodb, | ||
| ConnectionTypesEnum.cassandra, | ||
| ConnectionTypesEnum.agent_cassandra, | ||
| ConnectionTypesEnum.elasticsearch, | ||
| ]); |
Comment on lines
+109
to
+112
| await this._dbContext.tableSchemaChangeRepository.updateStatus(change.id, change.status, { | ||
| userModifiedSql, | ||
| }); | ||
| } |
Comment on lines
+33
to
+38
| if (connection.ssl) { | ||
| clientOptions.sslOptions = { rejectUnauthorized: false }; | ||
| if (connection.cert) { | ||
| clientOptions.sslOptions.ca = [connection.cert]; | ||
| } | ||
| } |
Comment on lines
+163
to
+170
| if (connection.ssl) { | ||
| const tls: Record<string, unknown> = { | ||
| rejectUnauthorized: !connection.cert, | ||
| }; | ||
| if (connection.cert) { | ||
| tls.ca = connection.cert; | ||
| } | ||
| options.tls = tls; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
backend/src/migrations/1777039182865-AddDynamoDbSchemaChangeTypes.ts-19-29 (1)
19-29:⚠️ Potential issue | 🟡 MinorThe
down()migration will fail if any rows have DynamoDB change types.When reverting, if any
table_schema_changerows havechangeTypevalues likeDYNAMODB_CREATE_TABLE, the cast on Line 24 will fail since those values don't exist in the recreated old enum. Consider either:
- Documenting that this migration is non-revertible if DynamoDB changes exist, or
- Adding a pre-check or data cleanup step before the enum revert.
This is acceptable if DynamoDB schema changes are not yet in production, but could cause issues during development rollbacks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/migrations/1777039182865-AddDynamoDbSchemaChangeTypes.ts` around lines 19 - 29, The down() migration can fail when reverting the enum because rows in table_schema_change may contain DynamoDB values (e.g., DYNAMODB_CREATE_TABLE) that are not present in the recreated "table_schema_change_changetype_enum_old"; before casting in down() (the ALTER TABLE ... USING ... on column changeType) add a safe cleanup or guard: either delete or map/convert any rows whose changeType is a DynamoDB-specific value to a supported fallback (or NULL) or bail with a clear error documenting non-revertible state; implement this check/cleanup in the migration before the ALTER TABLE or change the migration to be non-revertible by throwing/returning early from down() with an explanatory message. Ensure you modify the down() method in the migration that manipulates the "table_schema_change" table and the "table_schema_change_changetype_enum_old"/"table_schema_change_changetype_enum" enum names.backend/src/migrations/1776857063726-AddTableSchemaChangeEntity.ts-63-72 (1)
63-72:⚠️ Potential issue | 🟡 MinorThe
down()migration will fail if any rows use the new comparator values.If any
table_filtersrows havedynamic_filter_comparatorset to'in'or'between', the enum recast on Line 67 will fail. This is the same pattern as the DynamoDB migration issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/migrations/1776857063726-AddTableSchemaChangeEntity.ts` around lines 63 - 72, The down() migration in AddTableSchemaChangeEntity will fail if any table_filters.dynamic_filter_comparator rows contain the new values ('in' or 'between'); before changing the column type you must normalize those rows. In the down() method, prior to creating table_filters_dynamic_filter_comparator_enum_old and running the ALTER COLUMN, add a queryRunner.query(...) UPDATE that replaces new enum values with valid old ones (e.g. UPDATE "table_filters" SET "dynamic_filter_comparator" = 'contains' WHERE "dynamic_filter_comparator" IN ('in','between');), then proceed with creating the old enum, ALTER COLUMN, DROP the new enum and RENAME the old enum as currently implemented. Ensure you reference the same column name ("dynamic_filter_comparator") and enum names ("table_filters_dynamic_filter_comparator_enum_old", "table_filters_dynamic_filter_comparator_enum") when adding the UPDATE.backend/package.json-82-82 (1)
82-82:⚠️ Potential issue | 🟡 MinorConsider updating
node-sql-parserto version 5.4.0.The
node-sql-parserdependency is appropriate for DDL validation. However, version 5.4.0 was released on January 12, 2026. Update from^5.3.0to^5.4.0to use the latest stable version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/package.json` at line 82, Update the node-sql-parser dependency in package.json from "^5.3.0" to "^5.4.0" (change the value for the "node-sql-parser" entry), then run your package manager (npm install or yarn install) to refresh node_modules and update the lockfile (package-lock.json or yarn.lock); after upgrading, run the test/build commands to ensure DDL validation still passes and commit the updated lockfile with the package.json change.backend/src/entities/table-schema/utils/schema-change-ownership.guard.ts-26-29 (1)
26-29:⚠️ Potential issue | 🟡 MinorAdd defensive check for
request.decoded.If
request.decodedisundefined(e.g., middleware not run), accessing.subwill throw an unhandled error. Consider adding validation or relying on middleware ordering guarantees.🛡️ Proposed defensive check
async canActivate(context: ExecutionContext): Promise<boolean> { const request: IRequestWithCognitoInfo = context.switchToHttp().getRequest(); + if (!request.decoded?.sub) { + throw new BadRequestException('Authentication required.'); + } const userId = request.decoded.sub; const changeId: string = request.params?.changeId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/schema-change-ownership.guard.ts` around lines 26 - 29, The canActivate method currently reads request.decoded.sub without checking request.decoded; add a defensive check in canActivate (in file containing function canActivate and type IRequestWithCognitoInfo) to verify request?.decoded and request.decoded.sub exist before using them, and if missing either throw an UnauthorizedException (preferred) or return false so the guard fails safely; ensure changeId handling remains unchanged after the check.backend/src/entities/table-schema/utils/clickhouse-ddl.ts-14-20 (1)
14-20:⚠️ Potential issue | 🟡 MinorEmpty host fallback produces invalid URL.
When
connection.hostisnullorundefined, it defaults to an empty string, resulting in a malformed URL likehttp://:8123. This will cause a runtime error whencreateClientattempts to connect.Consider throwing a descriptive error for missing required fields:
🛡️ Proposed fix: Validate required host
export async function executeClickHouseDdl(connection: ClickHouseExecutionConnection, sql: string): Promise<void> { - const host = connection.host ?? ''; + const host = connection.host; + if (!host) { + throw new Error('ClickHouse connection requires a host'); + } const port = connection.port ?? 8123; const protocol = connection.ssl ? 'https' : 'http';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/clickhouse-ddl.ts` around lines 14 - 20, The URL construction in executeClickHouseDdl uses connection.host ?? '' which yields malformed URLs like "http://:8123" when host is missing; update the function to validate that connection.host is present and non-empty (and optionally that connection.port is a valid number) before building NodeClickHouseClientConfigOptions, and throw a descriptive error (e.g., "Missing ClickHouse host in connection") instead of proceeding to createClient so the caller receives a clear failure instead of a runtime URL/connection error.backend/src/migrations/1777295364595-AddBatchColumnsToTableSchemaChange.ts-12-15 (1)
12-15:⚠️ Potential issue | 🟡 MinorInconsistent schema qualification in index creation and deletion.
The
upmigration creates the index without schema prefix (CREATE INDEX "IDX_tsc_batch_order"), but thedownmigration drops it with explicit"public"schema (DROP INDEX "public"."IDX_tsc_batch_order"). This inconsistency violates the codebase pattern where other migrations consistently qualify indexes with the schema. Add the"public"schema prefix to theCREATE INDEXstatement in theupmigration to match the pattern and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/migrations/1777295364595-AddBatchColumnsToTableSchemaChange.ts` around lines 12 - 15, The CREATE INDEX in the up method is missing the "public" schema prefix while down drops "public"."IDX_tsc_batch_order"; update the up migration (method up in AddBatchColumnsToTableSchemaChange / function creating the index) to use CREATE INDEX "public"."IDX_tsc_batch_order" so it matches the DROP INDEX in down and follows the repository's schema qualification convention.backend/test/ava-tests/non-saas-tests/non-saas-table-schema-elasticsearch-e2e.test.ts-300-346 (1)
300-346:⚠️ Potential issue | 🟡 MinorThis test name overstates what it verifies.
The case says “rollback is recorded”, but it never calls
/rollbackand never asserts the rollback audit row. That leaves the ElasticsearchupdateMappingrollback behavior uncovered. Either rename the test to match the current assertions, or add the rollback call and audit checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-elasticsearch-e2e.test.ts` around lines 300 - 346, The test named "Elasticsearch: updateMapping adds a new field; rollback is recorded" does not actually exercise or assert rollback; either rename it to reflect only the approval/update behavior or add the missing rollback flow: after approving (function/test block around changeId and approveResp) POST to /table-schema/change/{changeId}/rollback with the same Cookie, assert a successful response, then query the audit/changes endpoint or DB to assert a rollback audit row exists and that the recorded rollbackOp matches the expected JSON (referencing nextProposal.rollbackOp and SchemaChangeTypeEnum.ELASTICSEARCH_UPDATE_MAPPING); ensure you also keep the final mapping assertion (getMapping(indexName)) as needed.backend/src/entities/table-schema/use-cases/reject-batch-schema-changes.use-case.ts-38-40 (1)
38-40:⚠️ Potential issue | 🟡 MinorBatch status updates are not wrapped in a transaction.
The loop updates each item's status individually. If an error occurs (e.g., database connectivity issue, constraint violation) after some items are updated, the batch will be left in an inconsistent state with some items rejected and others unchanged.
Consider wrapping the updates in a transaction to ensure atomicity.
💡 Suggested approach
If the repository supports transactions, wrap the updates:
await this._dbContext.connection.transaction(async (manager) => { for (const item of rejectable) { await this._dbContext.tableSchemaChangeRepository.updateStatus( item.id, SchemaChangeStatusEnum.REJECTED, manager ); } });Alternatively, add a
bulkUpdateStatusmethod to the repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/use-cases/reject-batch-schema-changes.use-case.ts` around lines 38 - 40, The loop in reject-batch-schema-changes.use-case.ts updates each item one-by-one using this._dbContext.tableSchemaChangeRepository.updateStatus and SchemaChangeStatusEnum.REJECTED, leaving the batch non-atomic if an error occurs; fix it by performing the updates inside a database transaction (e.g., use this._dbContext.connection.transaction and pass the transactional manager into updateStatus) or implement and call a repository-level bulkUpdateStatus that performs the status changes atomically. Ensure updateStatus accepts an optional transactional manager (or create bulkUpdateStatus) and wrap the for-loop so all rejections are committed or rolled back together.backend/src/migrations/1776953988756-AddMongoSchemaChangeTypes.ts-19-30 (1)
19-30:⚠️ Potential issue | 🟡 MinorThe
downmigration may fail if MongoDB change types exist in the database.The rollback casts
changeTypeto the old enum which excludesMONGO_*values. If any rows contain MongoDB-specific change types, theALTER TABLEat line 23-25 will fail with a cast error.Consider either:
- Documenting this limitation in a comment.
- Adding a pre-check or UPDATE to handle existing MongoDB rows (e.g., setting them to
OTHER).📝 Proposed documentation comment
public async down(queryRunner: QueryRunner): Promise<void> { + // WARNING: This rollback will fail if any table_schema_change rows have + // MONGO_* changeType values. Manually update or delete such rows first. await queryRunner.query( `CREATE TYPE "public"."table_schema_change_changetype_enum_old" AS ENUM('CREATE_TABLE', 'DROP_TABLE', 'ADD_COLUMN', 'DROP_COLUMN', 'ALTER_COLUMN', 'ADD_INDEX', 'DROP_INDEX', 'ADD_FOREIGN_KEY', 'DROP_FOREIGN_KEY', 'ADD_PRIMARY_KEY', 'DROP_PRIMARY_KEY', 'ROLLBACK', 'OTHER')`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/migrations/1776953988756-AddMongoSchemaChangeTypes.ts` around lines 19 - 30, The down migration in AddMongoSchemaChangeTypes.ts (public async down(queryRunner: QueryRunner)) will fail if rows contain MONGO_* enum values; before altering the column type, update any MongoDB-specific values to a safe value (e.g., set changeType = 'OTHER' for values starting with 'MONGO_') or add a clear comment documenting the limitation; modify the down method to run a pre-check/update via queryRunner.query to convert problematic changeType values to 'OTHER' (or skip/throw with a descriptive message) before executing the ALTER TABLE and DROP/RENAME TYPE statements.backend/test/ava-tests/non-saas-tests/non-saas-table-schema-mysql-e2e.test.ts-184-187 (1)
184-187:⚠️ Potential issue | 🟡 MinorAdd
clearAllTestKnex()to test teardown.The test creates Knex instances via
getTestKnex()which caches connections in an LRUCache. These cached connections are never explicitly destroyed becauseclearAllTestKnex()is not called in thetest.afterhook. While caching prevents connection pool exhaustion from repeated calls within tests, this omission leaves database connections unclosed at test completion. CallclearAllTestKnex()in the existingtest.afterhook to properly clean up all cached Knex instances.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-mysql-e2e.test.ts` around lines 184 - 187, The test opens cached Knex connections via getTestKnex but never clears them; update the existing test.after teardown to call clearAllTestKnex() so all cached Knex instances are destroyed and DB pools closed; locate the test.after hook in this file and add a call to clearAllTestKnex() (alongside any existing cleanup) to ensure proper teardown.backend/src/entities/table-schema/utils/validate-proposed-ddl.ts-160-162 (1)
160-162:⚠️ Potential issue | 🟡 MinorRegex issue in
normalizeIdentifier— may not correctly strip all quote styles.The regex
/^[\"[]|[`"]]$/g` has issues:
- The character class
[\"]]is malformed —]needs escaping as]` to be treated literally- The anchors
^and$combined with|mean it only matches quotes at the very start OR very end, not both in one pass- Identifiers like
"tableName"would becometableName"after one replaceProposed fix
function normalizeIdentifier(name: string): string { - return name.replace(/^[`"[]|[`"\]]$/g, '').toLowerCase(); + return name.replace(/^[`"\[]|[\]`"]$/g, '').toLowerCase(); }Or more robustly, strip matching quotes:
function normalizeIdentifier(name: string): string { let normalized = name; if ((normalized.startsWith('"') && normalized.endsWith('"')) || (normalized.startsWith('`') && normalized.endsWith('`')) || (normalized.startsWith('[') && normalized.endsWith(']'))) { normalized = normalized.slice(1, -1); } return normalized.toLowerCase(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/validate-proposed-ddl.ts` around lines 160 - 162, The normalizeIdentifier function uses a malformed regex (/^[`"[]|[`"\]]$/g) that won't reliably strip quoting and can leave a trailing quote; replace the regex approach with explicit logic in normalizeIdentifier that checks for and removes matching surrounding quote pairs (", `, and [ ])—e.g., if the string starts and ends with the same quote type (or '[' and ']'), slice off the first and last characters—and then return the result lowercased so identifiers like `"Table"` or `[Table]` become table.backend/src/entities/table-schema/utils/assert-dialect-supported.ts-32-34 (1)
32-34:⚠️ Potential issue | 🟡 MinorInconsistency:
isMongoDialectincludesagent_mongodbbutSUPPORTED_DIALECTSdoes not.The helper returns
trueforagent_mongodb, butassertDialectSupportedwould throw for that same connection type since it's not inSUPPORTED_DIALECTS. Ifagent_mongodbshould be supported, add it toSUPPORTED_DIALECTS. Otherwise, consider whether this helper should only check the supported subset.Proposed fix if agent_mongodb should be supported
const SUPPORTED_DIALECTS: ReadonlySet<ConnectionTypesEnum> = new Set([ ConnectionTypesEnum.postgres, ConnectionTypesEnum.mysql, ConnectionTypesEnum.mysql2, ConnectionTypesEnum.mssql, ConnectionTypesEnum.oracledb, ConnectionTypesEnum.ibmdb2, ConnectionTypesEnum.mongodb, + ConnectionTypesEnum.agent_mongodb, ConnectionTypesEnum.clickhouse, ConnectionTypesEnum.agent_clickhouse, ConnectionTypesEnum.dynamodb, ConnectionTypesEnum.cassandra, ConnectionTypesEnum.agent_cassandra, ConnectionTypesEnum.elasticsearch, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/assert-dialect-supported.ts` around lines 32 - 34, isMongoDialect currently returns true for ConnectionTypesEnum.agent_mongodb but SUPPORTED_DIALECTS does not include that enum, causing assertDialectSupported to reject agent_mongodb; fix by choosing one of two actions: (A) if agent_mongodb should be treated as supported, add ConnectionTypesEnum.agent_mongodb to SUPPORTED_DIALECTS and update any related tests/docs so assertDialectSupported and isMongoDialect agree, or (B) if agent_mongodb should not be supported, remove the agent_mongodb branch from isMongoDialect (so it only checks ConnectionTypesEnum.mongodb) and update any callers/tests that rely on isMongoDialect returning true for agent_mongodb. Ensure consistency between isMongoDialect, SUPPORTED_DIALECTS, and assertDialectSupported.
🧹 Nitpick comments (22)
backend/src/migrations/1776857063726-AddTableSchemaChangeEntity.ts (1)
6-54: Consider separating unrelated schema changes into distinct migrations.This migration bundles two logically unrelated changes:
- The
table_filters_dynamic_filter_comparator_enummodification (adding'in','between')- The new
table_schema_changeentity with its enums, table, and foreign keysCombining unrelated changes complicates debugging and selective rollbacks. If this is generated by TypeORM's migration:generate, consider splitting manually for cleaner version history.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/migrations/1776857063726-AddTableSchemaChangeEntity.ts` around lines 6 - 54, The migration mixes two unrelated changes: the enum/column change for table_filters_dynamic_filter_comparator_enum and the new table_schema_change entity (and its enums/constraints). Split this migration into two: keep queries that create/rename/alter table_filters_dynamic_filter_comparator_enum and the ALTER TABLE "table_filters" ALTER COLUMN call in one migration, and move the CREATE TYPEs for table_schema_change_databasetype_enum, table_schema_change_changetype_enum, table_schema_change_status_enum plus the CREATE TABLE "table_schema_change", its INDEXes and its ALTER TABLE … ADD CONSTRAINT foreign keys into a separate migration; update each migration’s up (and corresponding down) to only contain the relevant queries so symbols to locate are up(queryRunner: QueryRunner), "table_filters_dynamic_filter_comparator_enum", and "table_schema_change" (and its FK constraints FK_ab6fb01554213543f0a39f7d98e, FK_d4a735643602c7e2337770d368b, FK_f15e652a55b856bf9c64c012d00).backend/src/entities/table-schema/application/data-transfer-objects/schema-change-list-response.dto.ts (1)
2-2: Avoid cross-entity coupling for pagination contract.Line 2 imports
PaginationDsfrom the table entity package, which couples table-schema API responses to table internals. Consider using a shared/common pagination response DTO owned outside entity-specific modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/application/data-transfer-objects/schema-change-list-response.dto.ts` at line 2, The SchemaChangeListResponseDto currently imports PaginationDs from the table entity, creating cross-entity coupling; replace that with a shared/common pagination DTO: create or use an existing shared PaginationResponseDto (or similarly named DTO) owned by a common/shared package, update the import in schema-change-list-response.dto.ts to reference that shared DTO instead of PaginationDs, and adjust the SchemaChangeListResponseDto type references to use the new shared symbol (e.g., PaginationResponseDto) so table-schema is decoupled from table internals.backend/src/entities/table-schema/application/data-structures/get-schema-change.ds.ts (1)
1-4: Convert to an interface—this is a pure data transfer shape.This class is used exclusively as a type contract (never instantiated), making it a candidate for an interface per coding guidelines:
**/*.{ts,tsx}should use interfaces for object shapes.♻️ Suggested refactor
-export class GetSchemaChangeDs { - changeId: string; - userId: string; -} +export interface GetSchemaChangeDs { + changeId: string; + userId: string; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/application/data-structures/get-schema-change.ds.ts` around lines 1 - 4, Replace the class GetSchemaChangeDs with a TypeScript interface since it is only used as a type shape; change the declaration from "export class GetSchemaChangeDs { changeId: string; userId: string; }" to an exported interface named GetSchemaChangeDs with the same properties, and update any imports/usages (type-only imports if applicable) so consumers treat it as a type (no instantiation or "new") to satisfy the codebase convention of using interfaces for data-transfer shapes.backend/src/decorators/slug-uuid.decorator.ts (1)
6-40: Avoid duplicate sources of truth for allowed slug/UUID params.
SlugUuidParameterandavailableSlagParameterscurrently duplicate the same list. Consider deriving the union type from a singleas constarray to prevent future drift.♻️ Proposed refactor
-export type SlugUuidParameter = - | 'slug' - | 'connectionId' - | 'groupId' - | 'userId' - | 'actionId' - | 'ruleId' - | 'eventId' - | 'apiKeyId' - | 'companyId' - | 'threadId' - | 'filterId' - | 'chatId' - | 'changeId' - | 'batchId'; +const AVAILABLE_SLUG_UUID_PARAMETERS = [ + 'slug', + 'connectionId', + 'groupId', + 'userId', + 'apiKeyId', + 'actionId', + 'ruleId', + 'eventId', + 'companyId', + 'threadId', + 'filterId', + 'chatId', + 'changeId', + 'batchId', +] as const; +export type SlugUuidParameter = (typeof AVAILABLE_SLUG_UUID_PARAMETERS)[number]; @@ - const availableSlagParameters: Array<SlugUuidParameter> = [ - 'slug', - 'connectionId', - 'groupId', - 'userId', - 'apiKeyId', - 'actionId', - 'ruleId', - 'eventId', - 'companyId', - 'threadId', - 'filterId', - 'chatId', - 'changeId', - 'batchId', - ]; - if (!availableSlagParameters.includes(parameterName)) { + if (!AVAILABLE_SLUG_UUID_PARAMETERS.includes(parameterName)) { throw new BadRequestException(Messages.UUID_INVALID); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/decorators/slug-uuid.decorator.ts` around lines 6 - 40, The allowed slug/UUID values are duplicated between the SlugUuidParameter union and availableSlagParameters; refactor by extracting a single readonly array (e.g., const ALLOWED_SLUG_PARAMS = ['slug','connectionId',...] as const) and derive the type via type SlugUuidParameter = typeof ALLOWED_SLUG_PARAMS[number]; then update the SlugUuid decorator (createParamDecorator callback, parameterName default and the includes check) to use that single array (ALLOWED_SLUG_PARAMS) so the list of allowed keys is defined in one place.backend/src/entities/table-schema/utils/map-schema-change-to-response-dto.ts (1)
4-30: Convert to arrow function syntax to match project TypeScript style.This should be a
constarrow function per the repository's style preference for TypeScript files.♻️ Proposed refactor
-export function mapSchemaChangeToResponseDto(entity: TableSchemaChangeEntity): SchemaChangeResponseDto { - return { +export const mapSchemaChangeToResponseDto = ( + entity: TableSchemaChangeEntity, +): SchemaChangeResponseDto => { + return { id: entity.id, connectionId: entity.connectionId, batchId: entity.batchId, @@ appliedAt: entity.appliedAt, rolledBackAt: entity.rolledBackAt, }; -} +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/map-schema-change-to-response-dto.ts` around lines 4 - 30, Replace the named function mapSchemaChangeToResponseDto with a const arrow-function export to match project TypeScript style: declare export const mapSchemaChangeToResponseDto = (entity: TableSchemaChangeEntity): SchemaChangeResponseDto => { ... } (or use a concise object return), preserving all field mappings (id, connectionId, batchId, orderInBatch, authorId, previousChangeId, forwardSql, rollbackSql, userModifiedSql, status, changeType, targetTableName, databaseType, executionError, isReversible, autoRollbackAttempted, autoRollbackSucceeded, userPrompt, aiSummary, aiReasoning, createdAt, appliedAt, rolledBackAt) and the existing types TableSchemaChangeEntity and SchemaChangeResponseDto.backend/test/ava-tests/non-saas-tests/non-saas-table-schema-redis-e2e.test.ts (1)
67-67: Unused_testUtilsvariable.The
_testUtilsis assigned but never used in the test. Consider removing it if not needed, or add a comment explaining its intended future use.♻️ Remove if not needed
const mockFactory = new MockFactory(); let app: INestApplication; -let _testUtils: TestUtils; test.before(async () => { setSaasEnvVariable(); const moduleFixture = await Test.createTestingModule({ - imports: [ApplicationModule, DatabaseModule], - providers: [DatabaseService, TestUtils], + imports: [ApplicationModule, DatabaseModule], + providers: [DatabaseService], })And remove line 79:
- _testUtils = moduleFixture.get<TestUtils>(TestUtils);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-redis-e2e.test.ts` at line 67, Remove the unused variable declaration _testUtils of type TestUtils (it is declared but never referenced); either delete the line declaring let _testUtils: TestUtils; or, if you intend to use it later, add a brief comment above it explaining the planned purpose to avoid linter/test warnings—also ensure you remove the related redundant line mentioned in the review (the later unused assignment) so no dead/unused TestUtils symbols remain (search for _testUtils, TestUtils in the test file to remove or document consistently).backend/src/entities/table-schema/use-cases/list-schema-changes.use-case.ts (1)
30-32: Consider validating pagination inputs earlier.When
limit <= 0, the fallback toperPage = 1is defensive, but combined with a non-zerooffset, it produces potentially confusing pagination metadata (e.g.,limit=0, offset=50yieldscurrentPage=51, perPage=1).Consider validating and normalizing
limit/offsetat the controller or DTO level, or defaulting both to sensible values here (e.g.,limit = Math.max(1, limit),offset = Math.max(0, offset)).♻️ Optional: Add input normalization
protected async implementation(inputData: ListSchemaChangesDs): Promise<SchemaChangeListResponseDto> { - const { connectionId, limit, offset } = inputData; + const { connectionId } = inputData; + const limit = Math.max(1, inputData.limit); + const offset = Math.max(0, inputData.offset); const [items, total] = await this._dbContext.tableSchemaChangeRepository.findByConnectionPaginated(connectionId, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/use-cases/list-schema-changes.use-case.ts` around lines 30 - 32, Normalize and validate pagination inputs before computing perPage/currentPage/lastPage: ensure limit is at least 1 and offset is non-negative (e.g., set limit = Math.max(1, limit) and offset = Math.max(0, offset)) inside the list-schema-changes use case (or earlier in the controller/DTO), then compute perPage = limit, currentPage = Math.floor(offset / perPage) + 1 and lastPage = Math.max(1, Math.ceil(total / perPage)); update references to perPage/currentPage/lastPage in this file (list-schema-changes.use-case.ts) so pagination metadata is always sensible.backend/src/entities/table-schema/utils/schema-change-batch-ownership.guard.ts (1)
18-19: Move this guard into the guards layer.This is an authorization guard, but it lives under
entities/table-schema/utils. The repo guideline places guards insrc/guards/, so moving it there would keep the security layer discoverable and consistent. As per coding guidelines "Place authentication and authorization guards insrc/guards/directory".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/schema-change-batch-ownership.guard.ts` around lines 18 - 19, The SchemaChangeBatchOwnershipGuard class (implements CanActivate, decorated with `@Injectable`) should be moved from its current entities/table-schema/utils location into the guards layer (src/guards/); relocate the file, update any imports that reference SchemaChangeBatchOwnershipGuard, and update module provider registrations or exports where it is injected (e.g., any modules that list it in providers or use it in route guards) so the new path is used; ensure imports for CanActivate and Injectable remain and run project tests/tsc to catch any remaining import paths to update.backend/test/ava-tests/non-saas-tests/non-saas-table-schema-ibmdb2-e2e.test.ts (1)
92-118: Replaceanyin the DB2 helpers.
params: anyandT = anyerase the contract for the connection config and query rows right where these tests rely on DB2-specific fields such asschema. A small DB2 connection interface plus a genericqueryDb2<T>without theanydefault keeps these helpers checked and easier to maintain. As per coding guidelines "Always add type annotations to function parameters and return types in TypeScript" and "Avoid any types - use specific types or generics instead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-ibmdb2-e2e.test.ts` around lines 92 - 118, Define a specific DB2 params interface (e.g. Db2Params with database, host, username, password, port, schema: string) and use it instead of any for buildConnStr(params: Db2Params), ensureSchema(params: Db2Params) and queryDb2<T>(params: Db2Params, sql: string): Promise<T[]>; remove the T = any default so callers must supply a concrete row type; update references to params.schema accordingly and ensure all three functions/signatures use the new Db2Params type and the precise return type Promise<T[]> for queryDb2.backend/test/ava-tests/non-saas-tests/non-saas-table-schema-mongodb-e2e.test.ts (1)
87-91: MissingbeforeEachhook to resetnextProposal.Similar to the MSSQL test file, this MongoDB test suite doesn't reset
nextProposalbetween tests, which could lead to test pollution.🧹 Proposed fix to add beforeEach reset
const createdCollections: string[] = []; +test.beforeEach(() => { + nextProposal = null; +}); + function buildMongoUri(params: any): string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-mongodb-e2e.test.ts` around lines 87 - 91, Add a beforeEach hook that resets the MockFactory's nextProposal counter to its initial value to avoid test pollution; specifically, add beforeEach(() => { mockFactory.nextProposal = 1; }); (or the same reset used in the MSSQL suite) near where mockFactory is declared so mockFactory.nextProposal is reset before each test run.backend/test/ava-tests/non-saas-tests/non-saas-table-schema-oracle-e2e.test.ts (2)
89-92: MissingbeforeEachhook to resetnextProposal.Consistent with the other test files, consider adding a
beforeEachhook to resetnextProposalto prevent test pollution.🧹 Proposed fix to add beforeEach reset
const testTables: Array<string> = []; +test.beforeEach(() => { + nextProposal = null; +}); + test.before(async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-oracle-e2e.test.ts` around lines 89 - 92, Tests are missing a beforeEach hook to reset the shared nextProposal state, which can leak between tests; add a beforeEach that sets nextProposal = 0 (or its initial value) before each test run to mirror other test files — locate the tests in non-saas-table-schema-oracle-e2e.test.ts and add the hook near the top where mockFactory, app, _testUtils, and testTables are defined so nextProposal is reset prior to each it/test.
1-285: Consider expanding Oracle test coverage.The Oracle test suite has notably fewer tests compared to MySQL and MSSQL. Missing coverage includes:
- Destructive operations requiring
confirmedDestructive- Invalid SQL handling and
FAILEDstatususerModifiedSqlvalidation- Index and foreign key operations
- Rollback state constraints
This may be intentional if Oracle is a lower-priority backend, but consider adding parity tests for critical paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-oracle-e2e.test.ts` around lines 1 - 285, Add parity tests for Oracle by creating new serial tests (using createConnection, oracleKnex, randomOracleTableName and nextProposal) that mirror the MySQL/MSSQL coverage: 1) a destructive change test that sends a proposal with destructive SQL and asserts the API requires confirmedDestructive before approve, 2) an invalid-SQL test that returns FAILED status when the forwardSql is invalid, 3) a userModifiedSql validation test that posts a change with invalid userModifiedSql and asserts validation error, 4) index and foreign-key operation tests (CREATE/DROP INDEX, ADD/DROP CONSTRAINT) that verify the schema changes apply/rollback, and 5) rollback state constraint tests that attempt rollback when not allowed and assert proper error/status; for each test use the same endpoints (/table-schema/{connectionId}/generate, /table-schema/change/{id}/approve, /rollback) and oracle helper oracleKnex()/oracleColumnExists() to verify DB state.backend/test/ava-tests/non-saas-tests/non-saas-table-schema-mssql-e2e.test.ts (1)
89-92: MissingbeforeEachhook to resetnextProposal.Unlike the MySQL test file which resets
nextProposalintest.beforeEach, this file doesn't reset the test state between tests. If a test fails to setnextProposal, it could inadvertently use a stale value from a previous test, leading to confusing failures.🧹 Proposed fix to add beforeEach reset
const testTables: Array<string> = []; +test.beforeEach(() => { + nextProposal = null; +}); + test.before(async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-mssql-e2e.test.ts` around lines 89 - 92, Add a test.beforeEach hook to reset the shared nextProposal state before each test to avoid cross-test contamination; locate the test suite setup near the MockFactory/app/_testUtils/testTables declarations (symbols: mockFactory, app, _testUtils, testTables) and add a beforeEach that sets nextProposal back to a neutral value (e.g., undefined or null) so each test starts with a clean slate.backend/test/ava-tests/non-saas-tests/non-saas-table-schema-dynamodb-e2e.test.ts (1)
541-578: Missing cleanup registration for test tables.The test at line 541 (
userModifiedSql with mismatched tableName is rejected) does not addtableNametocreatedTables. If the test fails unexpectedly or the table gets created due to a bug, it won't be cleaned up. The same issue exists in tests starting at lines 580 and 607.🧹 Proposed fix to ensure cleanup
test.serial('DynamoDB: userModifiedSql with mismatched tableName is rejected', async (t) => { const { token } = await registerUserAndReturnUserInfo(app); const connectionId = await createConnection(token); const tableName = randomTableName('ra_ddb_umbad'); + createdTables.push(tableName); nextProposal = {Apply similar fixes for tests at lines 580 and 607.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-dynamodb-e2e.test.ts` around lines 541 - 578, The test "DynamoDB: userModifiedSql with mismatched tableName is rejected" fails to register the generated table for cleanup—ensure you push the generated tableName into the shared createdTables array right after you generate tableName (the variable created by randomTableName) so any accidental table creation is torn down; locate the test function using symbols registerUserAndReturnUserInfo, createConnection, randomTableName and nextProposal and add createdTables.push(tableName). Apply the same change to the other two tests referenced (the ones that follow, using the same pattern) so each test that defines a tableName adds it to createdTables for cleanup.backend/src/entities/table-schema/utils/execute-schema-change.ts (1)
34-43: Inconsistent dispatch: MongoDB only checkschangeType, unlike DynamoDB/Elasticsearch which check both.The DynamoDB (Line 45) and Elasticsearch (Line 56) branches check
changeType OR connectionType, but MongoDB only checkschangeType. If a MongoDB connection receives a non-MongochangeType(e.g., due to miscategorization), it would incorrectly fall through to the SQL path and fail.Consider adding a
isMongoDialect(connectionType)check for consistency:Proposed fix
- if (isMongoSchemaChangeType(changeType)) { + if (isMongoSchemaChangeType(changeType) || isMongoDialect(connectionType)) { const op = validateProposedMongoOp({Note: You'll need to import
isMongoDialectfrom./assert-dialect-supported.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/execute-schema-change.ts` around lines 34 - 43, The Mongo branch currently dispatches solely on isMongoSchemaChangeType which can let non-Mongo connections be treated as Mongo; update the dispatch to also verify the connection dialect by checking isMongoDialect(connectionType) (importing isMongoDialect from ./assert-dialect-supported.js) so the block becomes guarded by both isMongoSchemaChangeType(changeType) && isMongoDialect(connectionType); keep the same internals (validateProposedMongoOp, executeMongoSchemaOp) and return behavior unchanged.backend/src/entities/table-schema/use-cases/rollback-schema-change.use-case.ts (1)
104-104: Non-null assertion onupdated— handle potential null return.
updateStatuscan returnnullper the interface. While unlikely after a successful findByIdWithRelations, defensive handling is safer.Proposed fix
- return mapSchemaChangeToResponseDto(updated!); + if (!updated) { + throw new NotFoundException('Schema change not found after status update.'); + } + return mapSchemaChangeToResponseDto(updated);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/use-cases/rollback-schema-change.use-case.ts` at line 104, The code currently uses a non-null assertion on updated when returning mapSchemaChangeToResponseDto(updated!), but updateStatus can legally return null; modify the rollback flow in rollbackSchemaChangeUseCase (after calling updateStatus) to defensively check for a null return from updateStatus (variable updated) and handle it — e.g., throw a descriptive error or return a 404/appropriate error response — before calling mapSchemaChangeToResponseDto; reference the updateStatus call site, the updated variable, the mapSchemaChangeToResponseDto function, and the earlier findByIdWithRelations call to ensure consistent error handling.backend/src/entities/table-schema/utils/validate-proposed-ddl.ts (1)
86-89: Potential false positive: semicolon check doesn't account for string literals or comments.The check
if (/;/.test(stripped))may incorrectly reject valid single-statement DDL containing semicolons inside string literals (e.g.,DEFAULT ';') or comments. Consider this an edge case to monitor — the defense-in-depth approach with parser fallback mitigates risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/validate-proposed-ddl.ts` around lines 86 - 89, The current multi-statement detection using stripped = trimmed.replace(/;+\s*$/, '') and if (/;/.test(stripped)) can false-positive on semicolons inside string literals or comments; update the check in validate-proposed-ddl (validate-proposed-ddl.ts) to ignore semicolons that occur inside SQL string literals or comments before deciding it's multi-statement — implement a lightweight tokenizer that walks trimmed and strips/ignores single-quoted, double-quoted, dollar-quoted strings and SQL comments (-- ... and /* ... */), then test the remaining text for semicolons, or reuse the existing parser fallback path if tokenization fails; ensure you still trim only trailing statement terminators before this tokenization to preserve legitimate trailing semicolon handling.backend/src/entities/table-schema/utils/mongo-schema-op.ts (1)
232-261: Add exhaustive check for unhandled MongoDB operation kinds.Consistent with the other schema operation utilities, adding a
defaultcase improves type safety.🛡️ Proposed fix
async function dispatchMongoOp(db: Db, op: MongoSchemaOp): Promise<void> { switch (op.operation) { case 'createCollection': { await db.createCollection(op.collectionName); return; } case 'dropCollection': { await db.collection(op.collectionName).drop(); return; } case 'setValidator': { const command: Document = { collMod: op.collectionName }; command.validator = op.validatorSchema ?? {}; command.validationLevel = op.validationLevel ?? 'strict'; command.validationAction = op.validationAction ?? 'error'; await db.command(command); return; } case 'createIndex': { if (!op.indexSpec) throw new BadRequestException('createIndex is missing indexSpec.'); await db.collection(op.collectionName).createIndex(op.indexSpec, op.indexOptions ?? {}); return; } case 'dropIndex': { if (!op.indexName) throw new BadRequestException('dropIndex is missing indexName.'); await db.collection(op.collectionName).dropIndex(op.indexName); return; } + default: { + const _exhaustive: never = op.operation; + throw new Error(`Unhandled MongoDB operation: ${_exhaustive}`); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/mongo-schema-op.ts` around lines 232 - 261, The switch in dispatchMongoOp handling MongoSchemaOp is missing a default branch for unhandled operation kinds; add an exhaustive default case at the end of the switch in dispatchMongoOp that throws a clear error (e.g., BadRequestException or a similar runtime exception) including the unexpected op.operation value and possibly the op object for debugging to ensure any new/unknown MongoSchemaOp variants surface as failures rather than silently ignored.backend/src/entities/table-schema/utils/elasticsearch-schema-op.ts (1)
175-201: Add exhaustive check for unhandled operation kinds.The
dispatchElasticsearchOpfunction's switch statement lacks adefaultcase. WhilevalidateProposedElasticsearchOpensures only valid operations reach this point, adding an exhaustive check improves type safety and catches potential future additions.🛡️ Proposed fix
async function dispatchElasticsearchOp(client: Client, op: ElasticsearchSchemaOp): Promise<void> { switch (op.operation) { case 'createIndex': { await client.indices.create({ index: op.indexName, body: { ...(op.mappings ? { mappings: op.mappings } : {}), ...(op.settings ? { settings: op.settings } : {}), }, } as Parameters<typeof client.indices.create>[0]); return; } case 'deleteIndex': { await client.indices.delete({ index: op.indexName }); return; } case 'updateMapping': { if (!op.properties) { throw new BadRequestException('updateMapping is missing properties.'); } await client.indices.putMapping({ index: op.indexName, body: { properties: op.properties }, } as Parameters<typeof client.indices.putMapping>[0]); return; } + default: { + const _exhaustive: never = op.operation; + throw new Error(`Unhandled Elasticsearch operation: ${_exhaustive}`); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/elasticsearch-schema-op.ts` around lines 175 - 201, The switch in dispatchElasticsearchOp does not handle unknown op.operation kinds—add an exhaustive default branch that throws a runtime error (or uses a utility like assertUnreachable) when encountering an unexpected operation to make the function fail-fast and preserve type safety; update dispatchElasticsearchOp to include a default that references the incoming op.operation in the thrown message and mention validateProposedElasticsearchOp in a comment if desired so future changes to operation kinds are caught here.backend/src/entities/table-schema/table-schema.controller.ts (1)
42-47: Remove redundant@Injectable()decorator from controller.Controllers in NestJS are automatically injectable. The
@Injectable()decorator is redundant when used with@Controller()and may cause confusion about the class's role.♻️ Proposed fix
`@UseInterceptors`(SentryInterceptor) `@Controller`() `@ApiBearerAuth`() `@ApiTags`('Table Schema Changes') -@Injectable() export class TableSchemaController {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/table-schema.controller.ts` around lines 42 - 47, Remove the redundant `@Injectable`() decorator from the TableSchemaController class: locate the class declaration for TableSchemaController that is annotated with `@Controller`(), `@UseInterceptors`(SentryInterceptor), `@ApiBearerAuth`(), and `@ApiTags`('Table Schema Changes') and delete the `@Injectable`() line so the controller relies on Nest's automatic injectability provided by `@Controller`().backend/src/entities/table-schema/utils/dynamodb-schema-op.ts (1)
222-278: Add exhaustive check for unhandled DynamoDB operation kinds.Similar to the Elasticsearch utility, the switch statement lacks a
defaultcase for type exhaustiveness.🛡️ Proposed fix
async function dispatchDynamoDbOp(client: DynamoDB, op: DynamoDbSchemaOp): Promise<void> { switch (op.operation) { case 'createTable': { // ... existing code ... return; } case 'deleteTable': { // ... existing code ... return; } case 'updateTable': { // ... existing code ... return; } case 'updateTimeToLive': { // ... existing code ... return; } + default: { + const _exhaustive: never = op.operation; + throw new Error(`Unhandled DynamoDB operation: ${_exhaustive}`); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/utils/dynamodb-schema-op.ts` around lines 222 - 278, The switch in dispatchDynamoDbOp (handling DynamoDbSchemaOp) is missing an exhaustive/default case; add a final default branch (or an assertUnreachable) that throws a clear error (e.g., BadRequestException or Error) when op.operation is an unknown/unsupported value so unhandled operation kinds fail loudly and the type switch is exhaustive.backend/src/entities/table-schema/use-cases/approve-and-apply-schema-change.use-case.ts (1)
131-136: Consider using optimistic locking for APPLYING → APPLIED transition.The
updateStatuscall at line 131-135 doesn't verify the expected prior status (APPLYING). If a concurrent process somehow modified the status (e.g., a timeout mechanism marking it FAILED), this could overwrite that state. The code already usestransitionStatusIfMatchesat line 114 for the initial transition; consider using it here for consistency.🔒 Proposed fix
- const updated = await this._dbContext.tableSchemaChangeRepository.updateStatus( + const updated = await this._dbContext.tableSchemaChangeRepository.transitionStatusIfMatches( change.id, - SchemaChangeStatusEnum.APPLIED, - { appliedAt: new Date(), executionError: null }, + SchemaChangeStatusEnum.APPLYING, + SchemaChangeStatusEnum.APPLIED, + { appliedAt: new Date(), executionError: null } ); + if (!updated) { + this.logger.warn(`Status changed concurrently after successful execution for change ${change.id}`); + } return mapSchemaChangeToResponseDto(updated!);Note: This depends on whether
transitionStatusIfMatchessupports additional fields. If not, a two-step approach (transition then update fields) may be needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/table-schema/use-cases/approve-and-apply-schema-change.use-case.ts` around lines 131 - 136, The updateStatus call on tableSchemaChangeRepository that sets SchemaChangeStatusEnum.APPLIED (used inside approve-and-apply-schema-change.use-case.ts) lacks an optimistic lock check for the prior APPLYING state and can overwrite concurrent state changes; replace this updateStatus(...) call with a call to transitionStatusIfMatches(change.id, SchemaChangeStatusEnum.APPLYING, SchemaChangeStatusEnum.APPLIED, { appliedAt: new Date(), executionError: null }) if transitionStatusIfMatches supports extra fields, or if not, perform a two-step safe sequence: first call transitionStatusIfMatches(change.id, SchemaChangeStatusEnum.APPLYING, SchemaChangeStatusEnum.APPLIED) and only when that returns success call the existing updateStatus to set appliedAt/executionError, then return mapSchemaChangeToResponseDto(updated) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbbd97a1-ab70-4b56-8c57-7c1aba501a49
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (70)
backend/package.jsonbackend/src/ai-core/tools/prompts.tsbackend/src/app.module.tsbackend/src/common/application/global-database-context.interface.tsbackend/src/common/application/global-database-context.tsbackend/src/common/data-injection.tokens.tsbackend/src/decorators/slug-uuid.decorator.tsbackend/src/entities/table-schema/ai/run-schema-change-ai-loop.tsbackend/src/entities/table-schema/ai/schema-change-prompts.tsbackend/src/entities/table-schema/ai/schema-change-tools.tsbackend/src/entities/table-schema/application/data-structures/approve-batch-schema-change.ds.tsbackend/src/entities/table-schema/application/data-structures/approve-schema-change.ds.tsbackend/src/entities/table-schema/application/data-structures/generate-schema-change.ds.tsbackend/src/entities/table-schema/application/data-structures/get-batch-schema-change.ds.tsbackend/src/entities/table-schema/application/data-structures/get-schema-change.ds.tsbackend/src/entities/table-schema/application/data-structures/list-schema-changes.ds.tsbackend/src/entities/table-schema/application/data-structures/reject-batch-schema-change.ds.tsbackend/src/entities/table-schema/application/data-structures/reject-schema-change.ds.tsbackend/src/entities/table-schema/application/data-structures/rollback-batch-schema-change.ds.tsbackend/src/entities/table-schema/application/data-structures/rollback-schema-change.ds.tsbackend/src/entities/table-schema/application/data-transfer-objects/approve-batch-schema-change.dto.tsbackend/src/entities/table-schema/application/data-transfer-objects/approve-schema-change.dto.tsbackend/src/entities/table-schema/application/data-transfer-objects/generate-schema-change.dto.tsbackend/src/entities/table-schema/application/data-transfer-objects/schema-change-batch-response.dto.tsbackend/src/entities/table-schema/application/data-transfer-objects/schema-change-list-response.dto.tsbackend/src/entities/table-schema/application/data-transfer-objects/schema-change-response.dto.tsbackend/src/entities/table-schema/repository/custom-table-schema-change-repository-extension.tsbackend/src/entities/table-schema/repository/table-schema-change.repository.interface.tsbackend/src/entities/table-schema/table-schema-change-enums.tsbackend/src/entities/table-schema/table-schema-change.entity.tsbackend/src/entities/table-schema/table-schema.controller.tsbackend/src/entities/table-schema/table-schema.module.tsbackend/src/entities/table-schema/use-cases/approve-and-apply-schema-change.use-case.tsbackend/src/entities/table-schema/use-cases/approve-batch-schema-changes.use-case.tsbackend/src/entities/table-schema/use-cases/generate-schema-change.use-case.tsbackend/src/entities/table-schema/use-cases/get-batch-schema-changes.use-case.tsbackend/src/entities/table-schema/use-cases/get-schema-change.use-case.tsbackend/src/entities/table-schema/use-cases/list-schema-changes.use-case.tsbackend/src/entities/table-schema/use-cases/reject-batch-schema-changes.use-case.tsbackend/src/entities/table-schema/use-cases/reject-schema-change.use-case.tsbackend/src/entities/table-schema/use-cases/rollback-batch-schema-changes.use-case.tsbackend/src/entities/table-schema/use-cases/rollback-schema-change.use-case.tsbackend/src/entities/table-schema/use-cases/table-schema-use-cases.interface.tsbackend/src/entities/table-schema/utils/assert-dialect-supported.tsbackend/src/entities/table-schema/utils/cassandra-ddl.tsbackend/src/entities/table-schema/utils/clickhouse-ddl.tsbackend/src/entities/table-schema/utils/dynamodb-schema-op.tsbackend/src/entities/table-schema/utils/elasticsearch-schema-op.tsbackend/src/entities/table-schema/utils/execute-schema-change.tsbackend/src/entities/table-schema/utils/map-schema-change-to-response-dto.tsbackend/src/entities/table-schema/utils/mongo-schema-op.tsbackend/src/entities/table-schema/utils/schema-change-batch-ownership.guard.tsbackend/src/entities/table-schema/utils/schema-change-ownership.guard.tsbackend/src/entities/table-schema/utils/validate-proposed-ddl.tsbackend/src/migrations/1776857063726-AddTableSchemaChangeEntity.tsbackend/src/migrations/1776953988756-AddMongoSchemaChangeTypes.tsbackend/src/migrations/1777039182865-AddDynamoDbSchemaChangeTypes.tsbackend/src/migrations/1777272567941-AddElasticsearchSchemaChangeTypes.tsbackend/src/migrations/1777295364595-AddBatchColumnsToTableSchemaChange.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-cassandra-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-clickhouse-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-dynamodb-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-elasticsearch-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-ibmdb2-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-mongodb-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-mssql-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-mysql-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-oracle-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-postgres-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-redis-e2e.test.ts
Comment on lines
+237
to
+242
| if (tc.name === GET_TABLE_STRUCTURE_TOOL_NAME) { | ||
| const tableName = tc.arguments.tableName as string; | ||
| if (!tableName) throw new Error('Missing argument "tableName"'); | ||
| const structure = await dao.getTableStructure(tableName, userEmail); | ||
| const foreignKeys = await dao.getTableForeignKeys(tableName, userEmail); | ||
| result = encodeToToon({ tableName, structure, foreignKeys }); |
There was a problem hiding this comment.
Do not let foreign-key introspection failure erase a successful structure lookup.
getTableForeignKeys() runs in the same success path as getTableStructure(). For MongoDB/DynamoDB/Elasticsearch or any DAO that does not support FK metadata, the whole inspection tool call turns into an error even though the structure fetch already succeeded, so the model loses the schema context it asked for.
Suggested fix
if (tc.name === GET_TABLE_STRUCTURE_TOOL_NAME) {
const tableName = tc.arguments.tableName as string;
if (!tableName) throw new Error('Missing argument "tableName"');
const structure = await dao.getTableStructure(tableName, userEmail);
- const foreignKeys = await dao.getTableForeignKeys(tableName, userEmail);
+ const foreignKeys = await dao.getTableForeignKeys(tableName, userEmail).catch(() => []);
result = encodeToToon({ tableName, structure, foreignKeys });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/table-schema/ai/run-schema-change-ai-loop.ts` around
lines 237 - 242, The code currently calls getTableForeignKeys() immediately
after getTableStructure(), which lets a foreign-key introspection failure erase
an otherwise-successful structure lookup; update the
GET_TABLE_STRUCTURE_TOOL_NAME branch (where getTableStructure,
getTableForeignKeys, and encodeToToon are used) to call getTableForeignKeys()
inside a try/catch (or otherwise handle its rejected promise), defaulting
foreignKeys to an empty array or null on error and preserving the successful
structure result, and optionally log the FK error for debugging before calling
encodeToToon({ tableName, structure, foreignKeys }).
Comment on lines
+41
to
+48
| const items = await this._dbContext.tableSchemaChangeRepository.findByBatchId(batchId); | ||
| if (items.length === 0) { | ||
| throw new NotFoundException('Schema change batch not found.'); | ||
| } | ||
|
|
||
| const pending = items.filter( | ||
| (it) => it.status === SchemaChangeStatusEnum.PENDING || it.status === SchemaChangeStatusEnum.APPROVED, | ||
| ); |
There was a problem hiding this comment.
Sort the batch explicitly by orderInBatch before filtering/executing.
This use case relies on repository return order for both apply and compensation. If findByBatchId() ever returns rows in a different order, dependent changes can run out of sequence and rollback will unwind the wrong subset in reverse.
Suggested fix
- const items = await this._dbContext.tableSchemaChangeRepository.findByBatchId(batchId);
+ const items = (
+ await this._dbContext.tableSchemaChangeRepository.findByBatchId(batchId)
+ ).sort((a, b) => a.orderInBatch - b.orderInBatch);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/entities/table-schema/use-cases/approve-batch-schema-changes.use-case.ts`
around lines 41 - 48, The code relies on repository ordering; sort the fetched
items explicitly by the numeric orderInBatch before any filtering or execution:
after calling tableSchemaChangeRepository.findByBatchId(batchId) (variable
items) perform a stable sort by item.orderInBatch (ascending) and then compute
pending from that sorted array (used later in the
approve-batch-schema-changes.use-case flow) so both apply and compensation run
in the correct sequence even if the repo returns rows in a different order.
Comment on lines
+93
to
+100
| const updated = await this._dbContext.tableSchemaChangeRepository.updateStatus( | ||
| item.id, | ||
| SchemaChangeStatusEnum.APPLIED, | ||
| { appliedAt: new Date(), executionError: null }, | ||
| ); | ||
| if (updated) { | ||
| applied.push(updated); | ||
| } |
There was a problem hiding this comment.
Treat a missing updateStatus() result as a failed state transition.
After the SQL has already run, both the APPLIED and ROLLED_BACK paths currently treat a falsy updateStatus() as success. That can leave the live schema changed while the row stays in the old status, and later compensation/audit logic will miss that item.
Suggested fix
const updated = await this._dbContext.tableSchemaChangeRepository.updateStatus(
item.id,
SchemaChangeStatusEnum.APPLIED,
{ appliedAt: new Date(), executionError: null },
);
- if (updated) {
- applied.push(updated);
- }
+ if (!updated) {
+ throw new Error(`Failed to persist APPLIED status for schema change ${item.id}`);
+ }
+ applied.push(updated);
@@
- await this._dbContext.tableSchemaChangeRepository.updateStatus(item.id, SchemaChangeStatusEnum.ROLLED_BACK, {
+ const rolledBack = await this._dbContext.tableSchemaChangeRepository.updateStatus(
+ item.id,
+ SchemaChangeStatusEnum.ROLLED_BACK,
+ {
rolledBackAt: new Date(),
- });
+ },
+ );
+ if (!rolledBack) {
+ throw new Error(`Failed to persist ROLLED_BACK status for schema change ${item.id}`);
+ }
successIds.push(item.id);Also applies to: 187-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/entities/table-schema/use-cases/approve-batch-schema-changes.use-case.ts`
around lines 93 - 100, The updateStatus() result must not be treated as success
when falsy: in approveBatchSchemaChanges use-case, change both APPLIED (where
you currently check const updated = await
this._dbContext.tableSchemaChangeRepository.updateStatus(...,
SchemaChangeStatusEnum.APPLIED, ...)) and the ROLLED_BACK path (the similar call
around lines 187-190) to treat a falsy updated as a failed state transition —
e.g., add the item to the failed/failedItems collection (or set
executionError/details) and do not push it to the applied/rolledBack arrays;
ensure you include a clear executionError message like "updateStatus returned no
result" so downstream compensation/audit will see the failure.
Comment on lines
+31
to
+39
| if (change.status !== SchemaChangeStatusEnum.PENDING) { | ||
| throw new ConflictException(`Schema change is ${change.status}; only PENDING changes can be rejected.`); | ||
| } | ||
|
|
||
| const updated = await this._dbContext.tableSchemaChangeRepository.updateStatus( | ||
| change.id, | ||
| SchemaChangeStatusEnum.REJECTED, | ||
| ); | ||
| return mapSchemaChangeToResponseDto(updated!); |
There was a problem hiding this comment.
Make the PENDING → REJECTED transition atomic.
This checks change.status and updates it in two separate steps. Two concurrent requests can both pass the PENDING check, and one can still overwrite a change that was approved or rejected in between. Please push the status predicate into the update itself, or wrap the read/update in a transaction and fail when no row matches the expected current state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/table-schema/use-cases/reject-schema-change.use-case.ts`
around lines 31 - 39, The current PENDING check and subsequent call to
tableSchemaChangeRepository.updateStatus(change.id,
SchemaChangeStatusEnum.REJECTED) are non-atomic and allow race conditions;
change the logic so the repository enforces the predicate (update where id = ?
and status = PENDING) and returns whether a row was updated, or wrap the
read+update in a transaction and throw a ConflictException if zero rows were
affected; update the use-case (reject-schema-change.use-case.ts) to call the new
repository method (or transaction wrapper) and handle the "no rows updated"
result by throwing the same ConflictException instead of assuming success.
Comment on lines
+40
to
+46
| const items = await this._dbContext.tableSchemaChangeRepository.findByBatchId(batchId); | ||
| if (items.length === 0) { | ||
| throw new NotFoundException('Schema change batch not found.'); | ||
| } | ||
|
|
||
| const applied = items.filter((it) => it.status === SchemaChangeStatusEnum.APPLIED); | ||
| if (applied.length === 0) { |
There was a problem hiding this comment.
Rollback order should be based on orderInBatch, not repository row order.
This loop assumes findByBatchId() already returns batch items in apply order, then reverses that array. If the repository returns a different ordering, rollbacks can run parent-before-child and fail on foreign-key/dependency constraints.
Suggested fix
- const items = await this._dbContext.tableSchemaChangeRepository.findByBatchId(batchId);
+ const items = (
+ await this._dbContext.tableSchemaChangeRepository.findByBatchId(batchId)
+ ).sort((a, b) => a.orderInBatch - b.orderInBatch);
@@
- const applied = items.filter((it) => it.status === SchemaChangeStatusEnum.APPLIED);
+ const applied = items.filter((it) => it.status === SchemaChangeStatusEnum.APPLIED);Also applies to: 71-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/entities/table-schema/use-cases/rollback-batch-schema-changes.use-case.ts`
around lines 40 - 46, The code assumes repository ordering; instead sort the
batch items by their orderInBatch field and use that sorted order to determine
rollback order. In rollback-batch-schema-changes.use-case.ts, after retrieving
items via this._dbContext.tableSchemaChangeRepository.findByBatchId(batchId)
filter for SchemaChangeStatusEnum.APPLIED as you already do, then sort the
applied array by eachItem.orderInBatch (numeric asc) and iterate it in reverse
(or sort desc) when calling the rollback logic; apply the same change to the
second place where items are iterated (the later applied loop around the
rollback calls). This ensures parent entries (lower orderInBatch) are rolled
back after their children and avoids FK/dependency failures.
Comment on lines
+17
to
+37
| const authProvider = new cassandra.auth.PlainTextAuthProvider( | ||
| connection.username ?? 'cassandra', | ||
| connection.password ?? 'cassandra', | ||
| ); | ||
|
|
||
| const clientOptions: cassandra.ClientOptions = { | ||
| contactPoints: [host], | ||
| localDataCenter: connection.dataCenter || undefined, | ||
| keyspace: connection.database ?? undefined, | ||
| authProvider, | ||
| protocolOptions: { port }, | ||
| queryOptions: { | ||
| consistency: cassandra.types.consistencies.localQuorum, | ||
| }, | ||
| }; | ||
|
|
||
| if (connection.ssl) { | ||
| clientOptions.sslOptions = { rejectUnauthorized: false }; | ||
| if (connection.cert) { | ||
| clientOptions.sslOptions.ca = [connection.cert]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f 'cassandra-ddl.ts' backend/Repository: rocket-admin/rocketadmin
Length of output: 125
🏁 Script executed:
cat -n backend/src/entities/table-schema/utils/cassandra-ddl.tsRepository: rocket-admin/rocketadmin
Length of output: 1692
Don't weaken Cassandra connection security by default.
This helper falls back to hardcoded cassandra / cassandra credentials when none are provided and disables TLS certificate verification with rejectUnauthorized: false. For a schema-execution path, that creates two production security risks: (1) misconfigured connections can succeed against unintended clusters as a superuser, and (2) MITM attacks become possible when SSL is enabled. Only create authProvider when both username and password are explicitly provided, and keep certificate validation enabled when TLS is requested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/table-schema/utils/cassandra-ddl.ts` around lines 17 -
37, The code currently creates a PlainTextAuthProvider with default
'cassandra'/'cassandra' and sets clientOptions.sslOptions.rejectUnauthorized =
false; change this so authProvider (cassandra.auth.PlainTextAuthProvider) is
only constructed and assigned to clientOptions.authProvider when both
connection.username and connection.password are explicitly provided (avoid
defaulting to 'cassandra'), and when connection.ssl is true set
clientOptions.sslOptions.rejectUnauthorized = true by default (keep TLS
certificate validation) while still attaching connection.cert to sslOptions.ca
if provided.
Comment on lines
+35
to
+47
| const member = await this.tableSchemaChangeRepository.findOne({ | ||
| where: { batchId }, | ||
| select: ['id', 'connectionId'], | ||
| }); | ||
| if (!member) { | ||
| throw new NotFoundException('Schema change batch not found.'); | ||
| } | ||
|
|
||
| const allowed = await this.cedarAuthService.validate({ | ||
| userId, | ||
| action: CedarAction.ConnectionEdit, | ||
| connectionId: member.connectionId, | ||
| }); |
There was a problem hiding this comment.
Authorize the whole batch, not an arbitrary member row.
findOne({ where: { batchId } }) checks permissions against whichever change row happens to be returned first. If a bad batch ever spans multiple connectionIds, a user with access to one connection could operate on changes from another. Please load the distinct connection IDs for the batch and reject unless the batch resolves to exactly one authorized connection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/entities/table-schema/utils/schema-change-batch-ownership.guard.ts`
around lines 35 - 47, Replace the single-row fetch with a query that loads all
distinct connectionId values for the given batchId (use
tableSchemaChangeRepository to select connectionId across rows), then: if no
connectionIds -> throw NotFoundException('Schema change batch not found.'); if
more than one distinct connectionId -> reject (throw BadRequestException or
ForbiddenException) because the batch spans multiple connections; otherwise take
the single connectionId and call cedarAuthService.validate({ userId, action:
CedarAction.ConnectionEdit, connectionId }) to authorize the whole batch
(references: tableSchemaChangeRepository, cedarAuthService.validate,
CedarAction.ConnectionEdit).
Comment on lines
+18
to
+24
| @Injectable() | ||
| export class SchemaChangeOwnershipGuard implements CanActivate { | ||
| constructor( | ||
| @InjectRepository(TableSchemaChangeEntity) | ||
| private readonly tableSchemaChangeRepository: Repository<TableSchemaChangeEntity>, | ||
| private readonly cedarAuthService: CedarAuthorizationService, | ||
| ) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check where other ownership/authorization guards are located
fd -t f -e ts guard backend/src/ | head -20Repository: rocket-admin/rocketadmin
Length of output: 919
Move guard to standard location.
Guards should be placed in backend/src/guards/ per project conventions. The current location (backend/src/entities/table-schema/utils/) deviates from the established pattern used by all other guards in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/table-schema/utils/schema-change-ownership.guard.ts`
around lines 18 - 24, The SchemaChangeOwnershipGuard class is in a nonstandard
location; move the SchemaChangeOwnershipGuard (the `@Injectable` class with
constructor injecting TableSchemaChangeEntity repository and
CedarAuthorizationService) into the project's standard guards directory where
other guard classes live, update all imports/usages to point to the new module
location, and ensure module/providers/registration (e.g., any module that
referenced the old file) are updated so the guard remains exported and
injectable with the same constructor dependencies.
Comment on lines
+19
to
+25
| public async down(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| `CREATE TYPE "public"."table_schema_change_changetype_enum_old" AS ENUM('ADD_COLUMN', 'ADD_FOREIGN_KEY', 'ADD_INDEX', 'ADD_PRIMARY_KEY', 'ALTER_COLUMN', 'CREATE_TABLE', 'DROP_COLUMN', 'DROP_FOREIGN_KEY', 'DROP_INDEX', 'DROP_PRIMARY_KEY', 'DROP_TABLE', 'DYNAMODB_CREATE_TABLE', 'DYNAMODB_DROP_TABLE', 'DYNAMODB_UPDATE_TABLE', 'DYNAMODB_UPDATE_TTL', 'MONGO_CREATE_COLLECTION', 'MONGO_CREATE_INDEX', 'MONGO_DROP_COLLECTION', 'MONGO_DROP_INDEX', 'MONGO_SET_VALIDATOR', 'OTHER', 'ROLLBACK')`, | ||
| ); | ||
| await queryRunner.query( | ||
| `ALTER TABLE "table_schema_change" ALTER COLUMN "changeType" TYPE "public"."table_schema_change_changetype_enum_old" USING "changeType"::"text"::"public"."table_schema_change_changetype_enum_old"`, | ||
| ); |
There was a problem hiding this comment.
Handle persisted Elasticsearch values before casting back to the old enum.
down() recreates an enum that does not include the three ELASTICSEARCH_* values, then immediately casts changeType into it. If any schema-change row already uses one of those values, this migration rollback will fail instead of downgrading cleanly. Please remap or purge those rows before the cast, or make the rollback explicitly reject when such data exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/migrations/1777272567941-AddElasticsearchSchemaChangeTypes.ts`
around lines 19 - 25, The down() migration must handle any persisted
ELASTICSEARCH_* values before casting to the old enum; modify the method (in the
migration class containing down and using queryRunner) to either (A) remap rows
in table_schema_change where changeType IN
('ELASTICSEARCH_CREATE_INDEX','ELASTICSEARCH_DROP_INDEX','ELASTICSEARCH_UPDATE_INDEX')
to a permitted enum value like 'OTHER' (using queryRunner.query UPDATE) before
the ALTER COLUMN, or (B) perform a SELECT COUNT(*) for those values and throw an
explicit error/exit if any exist so the rollback fails safely; ensure the
UPDATE/SELECT targets column "changeType" on table "table_schema_change" and
occurs before the ALTER TABLE ... ALTER COLUMN ... TYPE
"public"."table_schema_change_changetype_enum_old" USING ... statement.
Comment on lines
+194
to
+195
| const knex = getTestKnex(getTestData(mockFactory).connectionToPostgres); | ||
| t.true(await knex.schema.hasTable(tableName)); |
There was a problem hiding this comment.
Destroy per-test Knex clients.
This file repeatedly creates new pools with getTestKnex(...) and never closes them. Over the full suite that can leak connections and make teardown flaky. Register a teardown for each client or reuse a shared instance.
Suggested fix
const knex = getTestKnex(getTestData(mockFactory).connectionToPostgres);
+ t.teardown(async () => {
+ await knex.destroy();
+ });
t.true(await knex.schema.hasTable(tableName));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const knex = getTestKnex(getTestData(mockFactory).connectionToPostgres); | |
| t.true(await knex.schema.hasTable(tableName)); | |
| const knex = getTestKnex(getTestData(mockFactory).connectionToPostgres); | |
| t.teardown(async () => { | |
| await knex.destroy(); | |
| }); | |
| t.true(await knex.schema.hasTable(tableName)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/test/ava-tests/non-saas-tests/non-saas-table-schema-postgres-e2e.test.ts`
around lines 194 - 195, The test creates per-test Knex pools via
getTestKnex(getTestData(mockFactory).connectionToPostgres) but never closes
them; to fix, ensure each created knex client is destroyed after the test by
registering a teardown for the local knex instance (e.g., call knex.destroy() in
a t.teardown or afterEach hook immediately after creating the client) or
refactor to reuse a shared knex instance across tests; update the code around
the knex variable (the getTestKnex(...) call and its usages) to guarantee
knex.destroy() runs when the test finishes.
…ard for enhanced access control
…Docker Compose and test agent config
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/src/guards/schema-change-batch-ownership.guard.ts (1)
35-47:⚠️ Potential issue | 🟠 MajorAuthorize the whole batch, not an arbitrary member row.
findOne({ where: { batchId } })checks permissions against whichever batch row happens to come back first. If a batch ever contains changes from multipleconnectionIds, a user with access to one connection could operate on changes from another. Load the distinctconnectionIds for the batch, require exactly one, then authorize against that value.Suggested fix
- const member = await this.tableSchemaChangeRepository.findOne({ - where: { batchId }, - select: ['id', 'connectionId'], - }); - if (!member) { + const batchConnections = await this.tableSchemaChangeRepository + .createQueryBuilder('schema_change') + .select('DISTINCT schema_change.connectionId', 'connectionId') + .where('schema_change.batchId = :batchId', { batchId }) + .getRawMany<{ connectionId: string }>(); + if (batchConnections.length === 0) { throw new NotFoundException('Schema change batch not found.'); } + if (batchConnections.length > 1) { + throw new BadRequestException('Schema change batch spans multiple connections.'); + } const allowed = await this.cedarAuthService.validate({ userId, action: CedarAction.ConnectionEdit, - connectionId: member.connectionId, + connectionId: batchConnections[0].connectionId, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/guards/schema-change-batch-ownership.guard.ts` around lines 35 - 47, The guard currently authorizes using a single arbitrary row returned by tableSchemaChangeRepository.findOne which can pick a connectionId from any member; change it to load all distinct connectionId values for the given batchId (e.g., query tableSchemaChangeRepository for connectionId where batchId and dedupe) require that the resulting set has exactly one distinct connectionId (throw NotFoundException or a specific error if none, and a 400/Forbidden if multiple), and then call cedarAuthService.validate with that single connectionId and CedarAction.ConnectionEdit instead of using member.connectionId from findOne; update variable names (batchConnectionIds or distinctConnectionIds) and remove reliance on findOne to ensure the whole batch is authorized against a single connection.
🧹 Nitpick comments (3)
docker-compose.tst.yml (1)
269-273: Use env substitution instead of inline basic-auth credentials in healthcheck.Line 272 currently hardcodes
elastic:SuperSecretElasticPasswordin the command string. Prefer the service env var to reduce credential exposure in metadata and logs.🔧 Suggested change
- "curl -s -u elastic:SuperSecretElasticPassword http://localhost:9200/_cluster/health | grep -E '\"status\":\"(green|yellow)\"'", + "curl -fsS -u elastic:$$ELASTIC_PASSWORD http://localhost:9200/_cluster/health | grep -E '\"status\":\"(green|yellow)\"'",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.tst.yml` around lines 269 - 273, The healthcheck command under the test service currently hardcodes basic-auth credentials; change it to use environment substitution (e.g., ${ELASTIC_USERNAME} and ${ELASTIC_PASSWORD} or a single ${ELASTIC_CREDENTIALS}) so the curl -u value is not inline. Update the test service's env block to export those variables and replace the inline "elastic:SuperSecretElasticPassword" in the CMD-SHELL string with the interpolated vars (keeping the CMD-SHELL form and proper quoting) so credentials come from service environment rather than appearing directly in the healthcheck command.docker-compose.yml (1)
179-183: Avoid embedding credentials directly in the healthcheck command string.Line 182 includes inline basic auth credentials. Even in test stacks, this leaks into container metadata/process args and tends to trip secret scanners. Prefer using the existing env var in the command.
🔧 Suggested change
- "curl -s -u elastic:SuperSecretElasticPassword http://localhost:9200/_cluster/health | grep -E '\"status\":\"(green|yellow)\"'", + "curl -fsS -u elastic:$$ELASTIC_PASSWORD http://localhost:9200/_cluster/health | grep -E '\"status\":\"(green|yellow)\"'",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 179 - 183, The healthcheck 'test' command currently embeds the basic auth credentials inline; update the healthcheck under the 'test' entry to use the existing environment variable (e.g., ELASTIC_PASSWORD) instead of hardcoding the password by invoking curl with -u "elastic:$ELASTIC_PASSWORD" (wrap the curl in a shell invocation like sh -c or the equivalent array form so the container runtime expands the env var) so credentials come from the environment rather than appearing in the command string or process args.backend/src/guards/schema-change-ownership.guard.ts (1)
18-52: Consider sharing the schema-change authorization flow.This guard and
backend/src/guards/schema-change-batch-ownership.guard.tsnow duplicate request extraction, UUID validation, repository lookup, and Cedar authorization. A small shared helper/service would make future permission fixes land in one place and keep the error semantics aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/guards/schema-change-ownership.guard.ts` around lines 18 - 52, Both SchemaChangeOwnershipGuard and schema-change-batch-ownership.guard duplicate request extraction, UUID validation, repository lookup, and Cedar authorization; extract that logic into a shared service (e.g., SchemaChangeAuthorizationService) with a method like validateChangeOwnership(userId: string, changeId: string) that: validates the changeId (reusing ValidationHelper.isValidUUID), loads the TableSchemaChangeEntity (selecting id and connectionId) from the repository, calls cedarAuthService.validate({ userId, action: CedarAction.ConnectionEdit, connectionId }), and throws the same BadRequestException/NotFoundException/ForbiddenException as the guards currently do; update both guards' canActivate methods to call this new service method and return true on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/src/guards/schema-change-batch-ownership.guard.ts`:
- Around line 35-47: The guard currently authorizes using a single arbitrary row
returned by tableSchemaChangeRepository.findOne which can pick a connectionId
from any member; change it to load all distinct connectionId values for the
given batchId (e.g., query tableSchemaChangeRepository for connectionId where
batchId and dedupe) require that the resulting set has exactly one distinct
connectionId (throw NotFoundException or a specific error if none, and a
400/Forbidden if multiple), and then call cedarAuthService.validate with that
single connectionId and CedarAction.ConnectionEdit instead of using
member.connectionId from findOne; update variable names (batchConnectionIds or
distinctConnectionIds) and remove reliance on findOne to ensure the whole batch
is authorized against a single connection.
---
Nitpick comments:
In `@backend/src/guards/schema-change-ownership.guard.ts`:
- Around line 18-52: Both SchemaChangeOwnershipGuard and
schema-change-batch-ownership.guard duplicate request extraction, UUID
validation, repository lookup, and Cedar authorization; extract that logic into
a shared service (e.g., SchemaChangeAuthorizationService) with a method like
validateChangeOwnership(userId: string, changeId: string) that: validates the
changeId (reusing ValidationHelper.isValidUUID), loads the
TableSchemaChangeEntity (selecting id and connectionId) from the repository,
calls cedarAuthService.validate({ userId, action: CedarAction.ConnectionEdit,
connectionId }), and throws the same
BadRequestException/NotFoundException/ForbiddenException as the guards currently
do; update both guards' canActivate methods to call this new service method and
return true on success.
In `@docker-compose.tst.yml`:
- Around line 269-273: The healthcheck command under the test service currently
hardcodes basic-auth credentials; change it to use environment substitution
(e.g., ${ELASTIC_USERNAME} and ${ELASTIC_PASSWORD} or a single
${ELASTIC_CREDENTIALS}) so the curl -u value is not inline. Update the test
service's env block to export those variables and replace the inline
"elastic:SuperSecretElasticPassword" in the CMD-SHELL string with the
interpolated vars (keeping the CMD-SHELL form and proper quoting) so credentials
come from service environment rather than appearing directly in the healthcheck
command.
In `@docker-compose.yml`:
- Around line 179-183: The healthcheck 'test' command currently embeds the basic
auth credentials inline; update the healthcheck under the 'test' entry to use
the existing environment variable (e.g., ELASTIC_PASSWORD) instead of hardcoding
the password by invoking curl with -u "elastic:$ELASTIC_PASSWORD" (wrap the curl
in a shell invocation like sh -c or the equivalent array form so the container
runtime expands the env var) so credentials come from the environment rather
than appearing in the command string or process args.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8da3e98f-7bdf-459a-b364-26f461c976b2
📒 Files selected for processing (9)
backend/src/entities/table-schema/table-schema.controller.tsbackend/src/entities/table-schema/table-schema.module.tsbackend/src/guards/index.tsbackend/src/guards/schema-change-batch-ownership.guard.tsbackend/src/guards/schema-change-ownership.guard.tsbackend/test/mock.factory.tsdocker-compose.tst.ymldocker-compose.ymlrocketadmin-agent/.clickhouse_test_agent_config.txt
✅ Files skipped from review due to trivial changes (3)
- rocketadmin-agent/.clickhouse_test_agent_config.txt
- backend/src/guards/index.ts
- backend/test/mock.factory.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/entities/table-schema/table-schema.module.ts
This was referenced May 11, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes