Unify mission move across backend CLI and UI with project migration support#2124
Unify mission move across backend CLI and UI with project migration support#2124
Conversation
|
I changed the base branch to dev, as all new PRs must go to dev to test it properly on our integration instance of Kleinkram. @Idate96 can you resolve the merge conflicts, then I will do a code review and include your PR in the next release. Hopefully by the end of the week. |
|
Also, lets have a quick talk about this feature. |
Idate96
force-pushed
the
lorenzo/server-side-migration
branch
from
f8ddc4e to
fcbbd69
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Rebased onto I also aligned migration storage usage with the current Could you take another look when you have a moment? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@greptile |
Greptile SummaryThis PR unifies mission relocation into a single Key changes:
Minor issues noted:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| backend/src/services/mission.service.ts | Core moveMissions implementation added: bulk move with transactional DB update, case-insensitive collision detection, and best-effort storage tag rollback. Formatting-only changes elsewhere in the file. |
| backend/src/services/project.service.ts | New migrateProject method migrates all missions from source to target project with exact case-insensitive collision detection, optional archive-rename, and best-effort storage tag rollback. IStorageBucket injected into constructor. |
| backend/src/endpoints/auth/guards/mission.guards.ts | Replaced MoveMissionToProjectGuard (query-param based) with MoveMissionsByBodyGuard (body-based). Validates missionUUIDs array, targetProjectUUID UUID format, and newName single-mission restriction. Permission checks now run in parallel with Promise.all. |
| cli/kleinkram/api/routes.py | New _move_missions and _migrate_project route functions added with HTTP error mapping. The 404 project-not-found discrimination uses fragile message string-matching (flagged). |
| frontend/src/dialogs/modify-mission-location-dialog.vue | Dialog extended to support bulk mission moves. Cancel bug fixed (onDialogCancel instead of onDialogOK). isNoopMove uses some() to disable the OK button when any selected mission is already in the target, matching backend semantics. TypeScript type annotation mismatch noted for selectedMissions computed. |
| packages/api-dto/src/types/mission/move-missions.dto.ts | New MoveMissionsDto and MoveMissionsResponseDto. The response DTO is missing @Min(0) on movedMissionCount, unlike the parallel MigrateProjectResponseDto (flagged). |
Sequence Diagram
sequenceDiagram
participant C as Client (UI / CLI)
participant G as MoveMissionsByBodyGuard
participant MS as MissionService
participant DB as Database (Transaction)
participant ST as Storage (addTags)
C->>G: POST /missions/move {missionUUIDs[], targetProjectUUID, newName?}
G->>G: Validate UUIDs, newName restriction
G->>G: canAccessProject(target, CREATE)
G->>G: Promise.all canAccessMission(each, DELETE)
G-->>C: 400/401/403 if invalid/unauthorized
G->>MS: moveSelectedMissions(dto)
MS->>DB: BEGIN TRANSACTION
DB->>DB: findOne(targetProject)
DB->>DB: find(missions IN [...UUIDs])
DB->>DB: Validate uniqueness & no-op collision
DB->>DB: find(all missions in target project)
DB->>DB: Case-insensitive collision check
DB->>DB: missionRepository.update(project=target) × N
DB->>ST: addTags(file, {projectUuid: target}) × N files
ST-->>DB: tag success
DB->>DB: COMMIT
MS-->>C: {success, movedMissionCount, movedMissionUUIDs, targetProjectUUID}
note over DB,ST: On any failure: DB rolls back + best-effort tag rollback to source project
Prompt To Fix All With AI
This is a comment left during a code review.
Path: cli/kleinkram/api/routes.py
Line: 4036-4040
Comment:
**Fragile 404 source/target discrimination via message parsing**
`_migrate_project` distinguishes a source-not-found 404 from a target-not-found 404 by searching for the target UUID string inside the backend error message. This ties the CLI's error routing to the exact wording of the backend's `NotFoundException` — if that message is ever changed or if both UUIDs somehow appear in the same message, callers would receive a misleading "source project not found" error.
A more robust pattern would be to key off a structured error field (e.g., `"field": "targetProjectUUID"`) or to emit the same generic `ProjectNotFound` with the raw backend detail forwarded as the message:
```python
if resp.status_code == 404:
message = _extract_error_message(resp)
raise ProjectNotFound(message)
```
This avoids the string-matching heuristic entirely while still surfacing the backend's explanation.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: frontend/src/dialogs/modify-mission-location-dialog.vue
Line: 80-94
Comment:
**`selectedMissions` computed has a TypeScript type annotation mismatch**
The computed is annotated as `FlatMissionDto[]`, but when `properties.mission` (typed `MissionWithFilesDto | FlatMissionDto`) is a `MissionWithFilesDto`, the returned `[properties.mission]` is not structurally assignable to `FlatMissionDto[]`. `MissionWithFilesDto` extends `MissionWithCreatorDto` and adds `files: FileDto[]`, but it lacks `filesCount` and `size` which `FlatMissionDto` requires.
At runtime this causes no problems — the dialog only accesses `.uuid`, `.name`, and `.project.uuid`, which both types share — but the annotation is misleading. You can narrow the type of the computed to include both forms:
```ts
const selectedMissions = computed<(MissionWithFilesDto | FlatMissionDto)[]>(() => {
if (properties.missions && properties.missions.length > 0) {
return properties.missions;
}
if (properties.mission) {
return [properties.mission];
}
return [];
});
```
Downstream usages in this file (`mission.uuid`, `mission.name`, `mission.project.uuid`) will continue to resolve correctly because all three properties exist on `MissionWithCreatorDto`, the common base.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/api-dto/src/types/mission/move-missions.dto.ts
Line: 40-42
Comment:
**`movedMissionCount` missing `@Min(0)` validator**
`MigrateProjectResponseDto` (in the same PR) declares both `movedMissionCount` and `movedFileCount` with `@Min(0)`. `MoveMissionsResponseDto.movedMissionCount` omits this constraint, creating an inconsistency that could allow negative counts to pass deserialization in client code that validates responses.
```suggestion
@ApiProperty()
@IsInt()
@Min(0)
movedMissionCount!: number;
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (10): Last reviewed commit: "Merge origin/dev and address remaining P..." | Re-trigger Greptile
| if (apiKey) { | ||
| throw new UnauthorizedException('CLI Keys cannot migrate projects'); | ||
| } | ||
|
|
||
| const body = request.body as ProjectBody | undefined; | ||
| const sourceProjectUUID = body?.sourceProjectUUID; | ||
| const targetProjectUUID = body?.targetProjectUUID; | ||
|
|
||
| if (!sourceProjectUUID || !targetProjectUUID) { | ||
| throw new BadRequestException( | ||
| 'sourceProjectUUID and targetProjectUUID are required', | ||
| ); | ||
| } | ||
| if (!isUUID(sourceProjectUUID) || !isUUID(targetProjectUUID)) { | ||
| throw new BadRequestException( | ||
| 'sourceProjectUUID and targetProjectUUID must be valid UUIDs', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Inconsistent validation order between project and mission migration guards
MigrateProjectByBodyGuard checks API key authorization before validating the request body UUIDs:
if (apiKey) {
throw new UnauthorizedException('CLI Keys cannot migrate projects');
}
// ... then body validation at lines 216-225But MigrateMissionByBodyGuard (same PR) validates body first, then checks API key:
if (!missionUUID || !targetProjectUUID) {
throw new BadRequestException('...');
}
if (!isUUID(missionUUID) || !isUUID(targetProjectUUID)) {
throw new BadRequestException('...');
}
if (apiKey) {
throw new UnauthorizedException('CLI Keys cannot move missions');
}This causes an API-key caller with invalid/missing UUIDs to receive:
- 400 Bad Request for mission migration (body validated first)
- 401 Unauthorized for project migration (auth checked first)
For consistency and better security posture (avoid leaking "this endpoint blocks API keys" before validation), validate request body fields before auth shortcuts. Recommend reordering to match MigrateMissionByBodyGuard pattern: body validation → apiKey check → permission checks.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/endpoints/auth/guards/project.guards.ts
Line: 208-225
Comment:
**Inconsistent validation order between project and mission migration guards**
`MigrateProjectByBodyGuard` checks API key authorization before validating the request body UUIDs:
```ts
if (apiKey) {
throw new UnauthorizedException('CLI Keys cannot migrate projects');
}
// ... then body validation at lines 216-225
```
But `MigrateMissionByBodyGuard` (same PR) validates body first, then checks API key:
```ts
if (!missionUUID || !targetProjectUUID) {
throw new BadRequestException('...');
}
if (!isUUID(missionUUID) || !isUUID(targetProjectUUID)) {
throw new BadRequestException('...');
}
if (apiKey) {
throw new UnauthorizedException('CLI Keys cannot move missions');
}
```
This causes an API-key caller with invalid/missing UUIDs to receive:
- **400 Bad Request** for mission migration (body validated first)
- **401 Unauthorized** for project migration (auth checked first)
For consistency and better security posture (avoid leaking "this endpoint blocks API keys" before validation), validate request body fields before auth shortcuts. Recommend reordering to match `MigrateMissionByBodyGuard` pattern: body validation → apiKey check → permission checks.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Thanks — good catch on consistency. I aligned MigrateProjectByBodyGuard with the mission migration guard in 42dc87eb: body UUID presence/format is validated first, then API-key rejection is applied.
cli/kleinkram/api/routes.py
Outdated
| if resp.status_code == 409: | ||
| raise ProjectValidationError(f"project migration conflict for source={source_project_id} target={target_project_id}") |
There was a problem hiding this comment.
409 Conflict mapped to wrong error class
A 409 Conflict response (e.g., mission name collision, archive name taken) signals a resource state conflict, not a client-side validation error. However, it's mapped to ProjectValidationError:
if resp.status_code == 409:
raise ProjectValidationError(f"project migration conflict for ...")For comparison, the parallel mission migration handler (line 539) correctly uses a conflict-type error:
if resp.status_code == 409:
raise MissionExists(f"target project already has a mission with ...")Callers who catch ProjectValidationError will treat this as a bad request rather than recognizing it as a resource conflict. Consider raising a dedicated ProjectConflictError or reusing an existing conflict exception class so callers can surface clearer messaging (e.g., "migration failed because target project already has a mission named X").
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/kleinkram/api/routes.py
Line: 573-574
Comment:
**409 Conflict mapped to wrong error class**
A `409 Conflict` response (e.g., mission name collision, archive name taken) signals a **resource state conflict**, not a client-side validation error. However, it's mapped to `ProjectValidationError`:
```python
if resp.status_code == 409:
raise ProjectValidationError(f"project migration conflict for ...")
```
For comparison, the parallel mission migration handler (line 539) correctly uses a conflict-type error:
```python
if resp.status_code == 409:
raise MissionExists(f"target project already has a mission with ...")
```
Callers who catch `ProjectValidationError` will treat this as a bad request rather than recognizing it as a resource conflict. Consider raising a dedicated `ProjectConflictError` or reusing an existing conflict exception class so callers can surface clearer messaging (e.g., "migration failed because target project already has a mission named X").
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
I kept this mapping as-is in this PR. Backend project migration currently uses 409 for multiple conflict causes (name collisions and concurrent-state conflicts), and the CLI currently models these as ProjectValidationError. I prefer to keep one stable mapping here and introduce a dedicated ProjectConflictError in a follow-up if we want finer-grained handling.
Additional Comments (1)
This PR repositioned the admin early-return in three existing guards:
The admin check now runs after the missing-UUID validation. Previously, an ADMIN user could call these endpoints without supplying a If there are legitimate admin-only code paths (internal tooling, admin UI, existing tests) that call these endpoints without a body UUID, they will silently break after merge. This behavior change is not mentioned in the PR description. Action: Please confirm this stricter behavior for admins is intentional. If so, consider documenting it with a test or a code comment explaining why admins must now provide missionUUID. Prompt To Fix With AIThis is a comment left during a code review.
Path: backend/src/endpoints/auth/guards/mission.guards.ts
Line: 122-144
Comment:
**Admin bypass repositioned after UUID check in existing guards — silent behavior change**
This PR repositioned the admin early-return in three existing guards:
- `CreateInMissionByBodyGuard` (moved from line 124 to line 136)
- `WriteMissionByBodyGuard` (moved from line 158 to line 170)
- `CanDeleteMissionGuard` (moved from line 198 to line 215)
The admin check now runs **after** the missing-UUID validation. Previously, an ADMIN user could call these endpoints without supplying a `missionUUID` in the body and receive access (`true`). After this change, admins without missionUUID now receive denial (`return false` or `BadRequestException`).
If there are legitimate admin-only code paths (internal tooling, admin UI, existing tests) that call these endpoints without a body UUID, they will silently break after merge. This behavior change is not mentioned in the PR description.
**Action:** Please confirm this stricter behavior for admins is intentional. If so, consider documenting it with a test or a code comment explaining why admins must now provide missionUUID.
How can I resolve this? If you propose a fix, please make it concise. |
| export function CanMigrateMissionByBody() { | ||
| return applyDecorators( | ||
| SetMetadata('CanMigrateMissionByBody', true), | ||
| UseGuards(MigrateMissionByBodyGuard), | ||
| ApiResponse({ | ||
| status: 403, | ||
| type: ForbiddenException, | ||
| description: | ||
| 'User does not have permissions to migrate the specified mission.', | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
CanMigrateMissionByBody missing HTTP 401 documentation
MigrateMissionByBodyGuard.canActivate throws UnauthorizedException (HTTP 401) when an API key is detected:
if (apiKey) {
throw new UnauthorizedException('CLI Keys cannot move missions');
}But CanMigrateMissionByBody only documents status 403, not 401. This is inconsistent with the parallel CanMigrateProjectByBody decorator (line 148), which correctly documents 401. Clients or generated SDK consumers relying on the Swagger spec won't know to handle a 401 response.
| export function CanMigrateMissionByBody() { | |
| return applyDecorators( | |
| SetMetadata('CanMigrateMissionByBody', true), | |
| UseGuards(MigrateMissionByBodyGuard), | |
| ApiResponse({ | |
| status: 403, | |
| type: ForbiddenException, | |
| description: | |
| 'User does not have permissions to migrate the specified mission.', | |
| }), | |
| ); | |
| } | |
| export function CanMigrateMissionByBody() { | |
| return applyDecorators( | |
| SetMetadata('CanMigrateMissionByBody', true), | |
| UseGuards(MigrateMissionByBodyGuard), | |
| ApiResponse({ | |
| status: 401, | |
| type: UnauthorizedExceptionDto, | |
| description: | |
| 'User does not have permissions to migrate the specified mission.', | |
| }), | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/endpoints/auth/roles.decorator.ts
Line: 213-224
Comment:
`CanMigrateMissionByBody` missing HTTP 401 documentation
`MigrateMissionByBodyGuard.canActivate` throws `UnauthorizedException` (HTTP 401) when an API key is detected:
```ts
if (apiKey) {
throw new UnauthorizedException('CLI Keys cannot move missions');
}
```
But `CanMigrateMissionByBody` only documents status 403, not 401. This is inconsistent with the parallel `CanMigrateProjectByBody` decorator (line 148), which correctly documents 401. Clients or generated SDK consumers relying on the Swagger spec won't know to handle a 401 response.
```suggestion
export function CanMigrateMissionByBody() {
return applyDecorators(
SetMetadata('CanMigrateMissionByBody', true),
UseGuards(MigrateMissionByBodyGuard),
ApiResponse({
status: 401,
type: UnauthorizedExceptionDto,
description:
'User does not have permissions to migrate the specified mission.',
}),
);
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 42dc87eb. CanMigrateMissionByBody now documents 401 (API key not allowed) in addition to the existing 403 response.
|
Addressed review feedback in 42dc87e:
I intentionally kept CLI project migration 409 mapped to ProjectValidationError in this PR because backend returns 409 for multiple conflict causes; we can add a dedicated ProjectConflictError in a follow-up if we want finer conflict typing. |
| export function CanMigrateProjectByBody() { | ||
| return applyDecorators( | ||
| SetMetadata('CanMigrateProjectByBody', true), | ||
| UseGuards(MigrateProjectByBodyGuard), | ||
| ApiResponse({ | ||
| status: 401, | ||
| type: UnauthorizedExceptionDto, | ||
| description: | ||
| 'User does not have permissions to migrate between the specified projects.', | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
CanMigrateProjectByBody decorator is missing HTTP 403 documentation.
MigrateProjectByBodyGuard.canActivate can return false when canAccessProject evaluates to false for either the source or target project, which NestJS translates to a 403 Forbidden response. However, the decorator only documents the 401 case:
ApiResponse({ status: 401, ... })
// 403 is never declaredThe parallel decorator CanMigrateMissionByBody (lines 213-229) correctly documents both 401 and 403. Clients and generated SDK consumers relying on the Swagger spec won't know to handle a 403 response on POST /projects/migrate.
| export function CanMigrateProjectByBody() { | |
| return applyDecorators( | |
| SetMetadata('CanMigrateProjectByBody', true), | |
| UseGuards(MigrateProjectByBodyGuard), | |
| ApiResponse({ | |
| status: 401, | |
| type: UnauthorizedExceptionDto, | |
| description: | |
| 'User does not have permissions to migrate between the specified projects.', | |
| }), | |
| ); | |
| } | |
| export function CanMigrateProjectByBody() { | |
| return applyDecorators( | |
| SetMetadata('CanMigrateProjectByBody', true), | |
| UseGuards(MigrateProjectByBodyGuard), | |
| ApiResponse({ | |
| status: 401, | |
| type: UnauthorizedExceptionDto, | |
| description: | |
| 'User does not have permissions to migrate between the specified projects.', | |
| }), | |
| ApiResponse({ | |
| status: 403, | |
| type: ForbiddenException, | |
| description: | |
| 'User does not have sufficient access rights on the source or target project.', | |
| }), | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/endpoints/auth/roles.decorator.ts
Line: 148-159
Comment:
`CanMigrateProjectByBody` decorator is missing HTTP 403 documentation.
`MigrateProjectByBodyGuard.canActivate` can return `false` when `canAccessProject` evaluates to `false` for either the source or target project, which NestJS translates to a **403 Forbidden** response. However, the decorator only documents the 401 case:
```ts
ApiResponse({ status: 401, ... })
// 403 is never declared
```
The parallel decorator `CanMigrateMissionByBody` (lines 213-229) correctly documents both 401 and 403. Clients and generated SDK consumers relying on the Swagger spec won't know to handle a 403 response on `POST /projects/migrate`.
```suggestion
export function CanMigrateProjectByBody() {
return applyDecorators(
SetMetadata('CanMigrateProjectByBody', true),
UseGuards(MigrateProjectByBodyGuard),
ApiResponse({
status: 401,
type: UnauthorizedExceptionDto,
description:
'User does not have permissions to migrate between the specified projects.',
}),
ApiResponse({
status: 403,
type: ForbiddenException,
description:
'User does not have sufficient access rights on the source or target project.',
}),
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Idate96
changed the title
| const targetMissionNames = new Set( | ||
| targetMissions.map((m) => m.name), | ||
| ); | ||
| const collidingMissionNames = sourceMissions | ||
| .map((mission) => mission.name) | ||
| .filter((name) => targetMissionNames.has(name)); |
There was a problem hiding this comment.
The collision check here uses case-sensitive JavaScript Set.has(), but the parallel moveMissions method (same PR) uses case-insensitive DB queries via LOWER(name) = LOWER(:name).
This inconsistency means migrateProject will silently skip collisions between case variants (e.g., SomeMission vs somemission). If the DB has a case-insensitive unique constraint, the collision is caught later as a generic 23505 error without naming the offending missions.
For consistency and clearer error messages, apply the same case-insensitive normalization:
| const targetMissionNames = new Set( | |
| targetMissions.map((m) => m.name), | |
| ); | |
| const collidingMissionNames = sourceMissions | |
| .map((mission) => mission.name) | |
| .filter((name) => targetMissionNames.has(name)); | |
| const targetMissionNamesLower = new Set( | |
| targetMissions.map((m) => m.name.toLowerCase()), | |
| ); | |
| const collidingMissionNames = sourceMissions | |
| .map((mission) => mission.name) | |
| .filter((name) => | |
| targetMissionNamesLower.has(name.toLowerCase()), | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/services/project.service.ts
Line: 608-613
Comment:
The collision check here uses case-sensitive JavaScript `Set.has()`, but the parallel `moveMissions` method (same PR) uses case-insensitive DB queries via `LOWER(name) = LOWER(:name)`.
This inconsistency means `migrateProject` will silently skip collisions between case variants (e.g., `SomeMission` vs `somemission`). If the DB has a case-insensitive unique constraint, the collision is caught later as a generic `23505` error without naming the offending missions.
For consistency and clearer error messages, apply the same case-insensitive normalization:
```suggestion
const targetMissionNamesLower = new Set(
targetMissions.map((m) => m.name.toLowerCase()),
);
const collidingMissionNames = sourceMissions
.map((mission) => mission.name)
.filter((name) =>
targetMissionNamesLower.has(name.toLowerCase()),
);
```
How can I resolve this? If you propose a fix, please make it concise.
cli/kleinkram/core.py
Outdated
| - update_file | ||
| - update_mission | ||
| - update_project | ||
| - migrate_mission |
There was a problem hiding this comment.
The module docstring lists migrate_mission, but the actual function defined below (line 229) is move_missions. This will mislead readers scanning the docstring to discover available functionality.
| - migrate_mission | |
| - move_missions |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/kleinkram/core.py
Line: 10
Comment:
The module docstring lists `migrate_mission`, but the actual function defined below (line 229) is `move_missions`. This will mislead readers scanning the docstring to discover available functionality.
```suggestion
- move_missions
```
How can I resolve this? If you propose a fix, please make it concise.
| const archiveNameTaken = await projectRepository.exists({ | ||
| where: { | ||
| uuid: Not(sourceProjectUUID), | ||
| name: ILike(archiveSourceProjectAs), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The archive-name uniqueness check at line 591 uses ILike(archiveSourceProjectAs), which treats SQL wildcards _ (matches any single character) and % (matches any sequence) as special characters. A user archiving to old_backup would receive a spurious 409 conflict if a project named old backup, oldXbackup, etc. exists, even if no project named old_backup exists.
The parallel mission collision detection in mission.service.ts handles this correctly using LOWER(mission.name) = LOWER(:name), which is exact case-insensitive matching without wildcard interpretation. For consistency and correctness, apply the same pattern here:
| const archiveNameTaken = await projectRepository.exists({ | |
| where: { | |
| uuid: Not(sourceProjectUUID), | |
| name: ILike(archiveSourceProjectAs), | |
| }, | |
| }); | |
| const archiveNameTaken = await projectRepository | |
| .createQueryBuilder('project') | |
| .where('project.uuid != :sourceProjectUUID', { sourceProjectUUID }) | |
| .andWhere('LOWER(project.name) = LOWER(:name)', { | |
| name: archiveSourceProjectAs, | |
| }) | |
| .getExists(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/services/project.service.ts
Line: 588-593
Comment:
The archive-name uniqueness check at line 591 uses `ILike(archiveSourceProjectAs)`, which treats SQL wildcards `_` (matches any single character) and `%` (matches any sequence) as special characters. A user archiving to `old_backup` would receive a spurious 409 conflict if a project named `old backup`, `oldXbackup`, etc. exists, even if no project named `old_backup` exists.
The parallel mission collision detection in `mission.service.ts` handles this correctly using `LOWER(mission.name) = LOWER(:name)`, which is exact case-insensitive matching without wildcard interpretation. For consistency and correctness, apply the same pattern here:
```suggestion
const archiveNameTaken = await projectRepository
.createQueryBuilder('project')
.where('project.uuid != :sourceProjectUUID', { sourceProjectUUID })
.andWhere('LOWER(project.name) = LOWER(:name)', {
name: archiveSourceProjectAs,
})
.getExists();
```
How can I resolve this? If you propose a fix, please make it concise.
| targetMissionName, | ||
| ] of targetMissionNames) { | ||
| const exists = await missionRepository.exists({ | ||
| where: { | ||
| name: targetMissionName, | ||
| uuid: Not(missionUUID), | ||
| project: { uuid: targetProjectUUID }, | ||
| }, | ||
| }); | ||
| if (exists) { | ||
| throw new ConflictException( | ||
| `Mission name '${targetMissionName}' already exists in the target project`, |
There was a problem hiding this comment.
Collision check is case-sensitive, not case-insensitive
The PR description claims the collision check was "fixed from wildcard-prone ILike to exact case-insensitive LOWER(name)=LOWER(:name)", but the implementation uses plain TypeORM equality:
const exists = await missionRepository.exists({
where: {
name: targetMissionName, // ← case-sensitive in PostgreSQL
uuid: Not(missionUUID),
project: { uuid: targetProjectUUID },
},
});TypeORM translates this to WHERE name = $1, which is case-sensitive by default in PostgreSQL. If the target project has a mission named "MyMission", moving a mission named "mymission" would pass this check. The final outcome depends on whether the DB has a LOWER(name) unique constraint — if it doesn't, duplicates silently coexist; if it does, a generic 23505 error is caught without identifying the offending name.
This contradicts the self-collision check above (lines 514–523), which correctly normalizes to lowercase. Apply the same case-insensitive normalization here:
const exists = await missionRepository
.createQueryBuilder('mission')
.where('LOWER(mission.name) = LOWER(:name)', { name: targetMissionName })
.andWhere('mission.uuid != :uuid', { uuid: missionUUID })
.andWhere('mission.projectUuid = :projectUUID', { projectUUID: targetProjectUUID })
.getExists();Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/services/mission.service.ts
Line: 532-543
Comment:
**Collision check is case-sensitive, not case-insensitive**
The PR description claims the collision check was "fixed from wildcard-prone `ILike` to exact case-insensitive `LOWER(name)=LOWER(:name)`", but the implementation uses plain TypeORM equality:
```ts
const exists = await missionRepository.exists({
where: {
name: targetMissionName, // ← case-sensitive in PostgreSQL
uuid: Not(missionUUID),
project: { uuid: targetProjectUUID },
},
});
```
TypeORM translates this to `WHERE name = $1`, which is **case-sensitive** by default in PostgreSQL. If the target project has a mission named `"MyMission"`, moving a mission named `"mymission"` would pass this check. The final outcome depends on whether the DB has a `LOWER(name)` unique constraint — if it doesn't, duplicates silently coexist; if it does, a generic `23505` error is caught without identifying the offending name.
This contradicts the self-collision check above (lines 514–523), which correctly normalizes to lowercase. Apply the same case-insensitive normalization here:
```ts
const exists = await missionRepository
.createQueryBuilder('mission')
.where('LOWER(mission.name) = LOWER(:name)', { name: targetMissionName })
.andWhere('mission.uuid != :uuid', { uuid: missionUUID })
.andWhere('mission.projectUuid = :projectUUID', { projectUUID: targetProjectUUID })
.getExists();
```
How can I resolve this? If you propose a fix, please make it concise.| const result = await this.missionService.moveMissions( | ||
| dto.missionUUIDs, | ||
| dto.targetProjectUUID, | ||
| dto.newName, | ||
| ); | ||
| return { | ||
| success: true, | ||
| movedMissionCount: result.movedMissionCount, | ||
| targetProjectUUID: dto.targetProjectUUID, | ||
| }; |
There was a problem hiding this comment.
movedMissionUUIDs silently dropped from response
The service returns both movedMissionCount and movedMissionUUIDs, but the controller discards the UUIDs:
return {
success: true,
movedMissionCount: result.movedMissionCount,
targetProjectUUID: dto.targetProjectUUID,
// movedMissionUUIDs: result.movedMissionUUIDs ← never forwarded
};MoveMissionsResponseDto doesn't declare the field either, so callers (CLI, frontend) cannot programmatically know which UUIDs were actually moved in a bulk operation. The parallel MigrateProjectResponseDto does expose movedMissionUUIDs, making this an API inconsistency.
Add the field to both the DTO and the controller response:
// move-missions.dto.ts
export class MoveMissionsResponseDto {
@ApiProperty()
@IsBoolean()
success!: boolean;
@ApiProperty()
movedMissionCount!: number;
@ApiProperty({ type: [String] })
@IsArray()
@IsUUID('4', { each: true })
movedMissionUUIDs!: string[];
@ApiProperty()
targetProjectUUID!: string;
}// controller
return {
success: true,
movedMissionCount: result.movedMissionCount,
movedMissionUUIDs: result.movedMissionUUIDs,
targetProjectUUID: dto.targetProjectUUID,
};Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/endpoints/mission/mission.controller.ts
Line: 176-185
Comment:
**`movedMissionUUIDs` silently dropped from response**
The service returns both `movedMissionCount` and `movedMissionUUIDs`, but the controller discards the UUIDs:
```ts
return {
success: true,
movedMissionCount: result.movedMissionCount,
targetProjectUUID: dto.targetProjectUUID,
// movedMissionUUIDs: result.movedMissionUUIDs ← never forwarded
};
```
`MoveMissionsResponseDto` doesn't declare the field either, so callers (CLI, frontend) cannot programmatically know which UUIDs were actually moved in a bulk operation. The parallel `MigrateProjectResponseDto` **does** expose `movedMissionUUIDs`, making this an API inconsistency.
Add the field to both the DTO and the controller response:
```ts
// move-missions.dto.ts
export class MoveMissionsResponseDto {
@ApiProperty()
@IsBoolean()
success!: boolean;
@ApiProperty()
movedMissionCount!: number;
@ApiProperty({ type: [String] })
@IsArray()
@IsUUID('4', { each: true })
movedMissionUUIDs!: string[];
@ApiProperty()
targetProjectUUID!: string;
}
```
```ts
// controller
return {
success: true,
movedMissionCount: result.movedMissionCount,
movedMissionUUIDs: result.movedMissionUUIDs,
targetProjectUUID: dto.targetProjectUUID,
};
```
How can I resolve this? If you propose a fix, please make it concise.
cli/kleinkram/cli/_mission.py
Outdated
| mission_ids, mission_patterns = split_args(mission) | ||
| if mission_patterns and not source_project: |
There was a problem hiding this comment.
The aggregate split_args(mission) call assigns both mission_ids and mission_patterns, but mission_ids is never used—only mission_patterns is checked in the conditional. The actual per-mission resolution in the loop (line 188) calls split_args([mission_selector]) fresh. Remove the unused assignment:
| mission_ids, mission_patterns = split_args(mission) | |
| if mission_patterns and not source_project: | |
| _, mission_patterns = split_args(mission) | |
| if mission_patterns and not source_project: |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/kleinkram/cli/_mission.py
Line: 179-180
Comment:
The aggregate `split_args(mission)` call assigns both `mission_ids` and `mission_patterns`, but `mission_ids` is never used—only `mission_patterns` is checked in the conditional. The actual per-mission resolution in the loop (line 188) calls `split_args([mission_selector])` fresh. Remove the unused assignment:
```suggestion
_, mission_patterns = split_args(mission)
if mission_patterns and not source_project:
```
How can I resolve this? If you propose a fix, please make it concise.
cli/kleinkram/api/routes.py
Outdated
| if resp.status_code == 409: | ||
| raise MissionExists( | ||
| "target project already has a mission with one of the requested names: " | ||
| + ", ".join(str(mission_id) for mission_id in mission_ids) | ||
| ) |
There was a problem hiding this comment.
The 409 error message claims to show "requested names" but joins mission_ids (UUIDs), producing opaque errors. Users can't identify which name actually conflicts.
Since the backend error message should include the colliding name, consider forwarding that directly:
| if resp.status_code == 409: | |
| raise MissionExists( | |
| "target project already has a mission with one of the requested names: " | |
| + ", ".join(str(mission_id) for mission_id in mission_ids) | |
| ) | |
| if resp.status_code == 409: | |
| raise MissionExists(_extract_error_message(resp)) |
This leverages the backend's error detail instead of showing mission UUIDs to the user.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/kleinkram/api/routes.py
Line: 545-549
Comment:
The 409 error message claims to show "requested names" but joins `mission_ids` (UUIDs), producing opaque errors. Users can't identify which name actually conflicts.
Since the backend error message should include the colliding name, consider forwarding that directly:
```suggestion
if resp.status_code == 409:
raise MissionExists(_extract_error_message(resp))
```
This leverages the backend's error detail instead of showing mission UUIDs to the user.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
If this change is intentional (i.e., Loki is now optional), add a comment explaining the rationale. If callers in other services relied on the 409 response to detect Loki outages, they should be updated accordingly: Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/backend-common/src/modules/action-dispatcher/action-dispatcher.service.ts
Line: 110-118
Comment:
Loki unreachability is now downgraded from a fatal `ConflictException` to a warning log, allowing action dispatch to proceed without an audit trail. This is a significant behavioral change: actions will execute even when the logging backend is unavailable.
If this change is intentional (i.e., Loki is now optional), add a comment explaining the rationale. If callers in other services relied on the 409 response to detect Loki outages, they should be updated accordingly:
```suggestion
} catch {
// Loki is optional—log the issue but allow dispatch to continue.
// Actions will execute without audit trail until logging is restored.
this.logger.warn(
'Loki logging system is down or unreachable; continuing action dispatch without audit',
);
}
```
How can I resolve this? If you propose a fix, please make it concise. |
|
Final CI fix note (head
Fix applied:
After this, all PR checks are green ( |
| this.logger.error('Loki logging system is down or unreachable'); | ||
| throw new ConflictException( | ||
| 'Logging system (Loki) is not available. Please try again later.', | ||
| this.logger.warn( | ||
| 'Loki logging system is down or unreachable; continuing action dispatch', |
There was a problem hiding this comment.
this change is totally unrelated to this PR
There was a problem hiding this comment.
Removed. I dropped the unrelated change from this PR in bb2c3f1.
wp99cp
left a comment
wp99cp
left a comment
There was a problem hiding this comment.
please remove the unrelated changes
| @@ -164,12 +166,23 @@ export class MissionController { | |||
|
|
|||
| @Post('move') | |||
There was a problem hiding this comment.
@Idate96, can you explain the difference between this mission/move endpoint and the new project/migrate endpoint? For me, these seem to be identical in behaviour. Or did I miss something?
There was a problem hiding this comment.
The intent is:
- /mission/move: move explicitly selected mission(s) (missionUUIDs) to a target project; optional newName is only valid for a single mission.
- /projects/migrate: move all missions from a source project to a target project, with optional archive rename of the source project.
| const isNoopMove = computed<boolean>(() => { | ||
| if (!selectedProjectUuid.value || selectedMissions.value.length === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| return selectedMissions.value.every( | ||
| (mission) => mission.project.uuid === selectedProjectUuid.value, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
isNoopMove disables only for pure-noop batch, but backend rejects if ANY mission is already in the target
isNoopMove uses every() — so the OK button stays enabled as long as at least one selected mission is not already in the target project. However, the backend (moveMissions service, line 494) throws a ConflictException for each individual mission whose project.uuid === targetProject.uuid. This means a user moving a mixed batch (some missions already in the target, some not) will see the button enabled, click it, and receive an opaque 409 error.
For consistency with the backend's all-or-nothing semantics, the check should use some() instead:
| const isNoopMove = computed<boolean>(() => { | |
| if (!selectedProjectUuid.value || selectedMissions.value.length === 0) { | |
| return false; | |
| } | |
| return selectedMissions.value.every( | |
| (mission) => mission.project.uuid === selectedProjectUuid.value, | |
| ); | |
| }); | |
| const isNoopMove = computed<boolean>(() => { | |
| if (!selectedProjectUuid.value || selectedMissions.value.length === 0) { | |
| return false; | |
| } | |
| return selectedMissions.value.some( | |
| (mission) => mission.project.uuid === selectedProjectUuid.value, | |
| ); | |
| }); |
This way the button is disabled (and a tooltip can explain why) whenever any selected mission would conflict, matching what the backend actually enforces.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/dialogs/modify-mission-location-dialog.vue
Line: 80-88
Comment:
**`isNoopMove` disables only for pure-noop batch, but backend rejects if ANY mission is already in the target**
`isNoopMove` uses `every()` — so the OK button stays **enabled** as long as at least one selected mission is *not* already in the target project. However, the backend (`moveMissions` service, line 494) throws a `ConflictException` for each individual mission whose `project.uuid === targetProject.uuid`. This means a user moving a mixed batch (some missions already in the target, some not) will see the button enabled, click it, and receive an opaque 409 error.
For consistency with the backend's all-or-nothing semantics, the check should use `some()` instead:
```suggestion
const isNoopMove = computed<boolean>(() => {
if (!selectedProjectUuid.value || selectedMissions.value.length === 0) {
return false;
}
return selectedMissions.value.some(
(mission) => mission.project.uuid === selectedProjectUuid.value,
);
});
```
This way the button is disabled (and a tooltip can explain why) whenever any selected mission would conflict, matching what the backend actually enforces.
How can I resolve this? If you propose a fix, please make it concise.| for (const missionUUID of missionUUIDs) { | ||
| const canDeleteSourceMission = | ||
| await this.missionGuardService.canAccessMission( | ||
| user, | ||
| missionUUID, | ||
| AccessGroupRights.DELETE, | ||
| ); | ||
| if (!canDeleteSourceMission) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Sequential permission checks — N round-trips per mission
The guard awaits each canAccessMission call one-by-one in a for loop. For a bulk move of N missions this fires N sequential database queries before the request even reaches the service layer. While the impact is negligible for typical batch sizes (1–5 missions), it becomes a noticeable bottleneck as missionUUIDs.length grows.
Consider running the checks in parallel using Promise.all:
| for (const missionUUID of missionUUIDs) { | |
| const canDeleteSourceMission = | |
| await this.missionGuardService.canAccessMission( | |
| user, | |
| missionUUID, | |
| AccessGroupRights.DELETE, | |
| ); | |
| if (!canDeleteSourceMission) { | |
| return false; | |
| } | |
| } | |
| const results = await Promise.all( | |
| missionUUIDs.map((missionUUID) => | |
| this.missionGuardService.canAccessMission( | |
| user, | |
| missionUUID, | |
| AccessGroupRights.DELETE, | |
| ), | |
| ), | |
| ); | |
| if (results.some((ok) => !ok)) { | |
| return false; | |
| } |
This keeps the same all-or-nothing semantics while reducing latency from O(N) sequential round-trips to a single parallel fan-out.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/endpoints/auth/guards/mission.guards.ts
Line: 343-353
Comment:
**Sequential permission checks — N round-trips per mission**
The guard awaits each `canAccessMission` call one-by-one in a `for` loop. For a bulk move of N missions this fires N sequential database queries before the request even reaches the service layer. While the impact is negligible for typical batch sizes (1–5 missions), it becomes a noticeable bottleneck as `missionUUIDs.length` grows.
Consider running the checks in parallel using `Promise.all`:
```suggestion
const results = await Promise.all(
missionUUIDs.map((missionUUID) =>
this.missionGuardService.canAccessMission(
user,
missionUUID,
AccessGroupRights.DELETE,
),
),
);
if (results.some((ok) => !ok)) {
return false;
}
```
This keeps the same all-or-nothing semantics while reducing latency from O(N) sequential round-trips to a single parallel fan-out.
How can I resolve this? If you propose a fix, please make it concise.
|
Addressed the move/migrate collision issues in I reproduced three live regressions against the local PR stack before the fix:
Changes in this commit:
Verification on the running local stack after the patch:
I could not use the chrome-cdp skill here because the installed runtime wrapper |
| } | ||
| } | ||
| await this.missionRepository.update(uuid, { | ||
| name: name, | ||
| }); | ||
| return this.missionRepository.findOne({ where: { uuid } }); | ||
|
|
||
| for (const file of rollbackTags) { | ||
| await this.dataStorage.addTags(file.fileUUID, { | ||
| filename: file.filename, | ||
| missionUuid: file.missionUUID, | ||
| projectUuid: targetProjectUUID, | ||
| }); | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| if ( | ||
| error instanceof QueryFailedError && | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| error.driverError?.code === "23505" | ||
| ) { | ||
| throw new ConflictException( | ||
| "Mission with that name already exists in the target project", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Inside the dataSource.transaction callback, this.dataStorage.addTags(...) is called for every file (lines after the missionRepository.update loop). These storage calls are external HTTP/SDK calls that are not enrolled in the DB transaction. This means:
- The DB updates run → tags are then applied to storage (inside the callback, before TypeORM commits).
- If a storage call succeeds for file N but then the
dataSource.transactionmachinery fails to commit (e.g., transient DB error at commit time), the DB is rolled back, but the storage tags for file N already point to the target project — the source project data in the DB, the target project tag in storage.
The current catch block only runs when the callback itself rejects, not when the post-callback commit fails. If the commit-level error propagates through the try, the rollback loop would still run (good), but rollbackTags would already be fully populated, so the restoration would execute correctly.
This is a known limitation of non-atomic distributed operations and the best-effort rollback mitigates it. However, the code comment that says "best-effort rollback" only appears in the catch, not at the storage-call site, which may surprise a future maintainer. Consider adding a short inline comment before the addTags loop clarifying that these calls are intentionally made inside the callback so that a storage failure rolls back the DB.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/services/mission.service.ts
Line: 555-575
Comment:
**Tag-update success window before DB commit creates orphaned storage state**
Inside the `dataSource.transaction` callback, `this.dataStorage.addTags(...)` is called for every file (lines after the `missionRepository.update` loop). These storage calls are external HTTP/SDK calls that are not enrolled in the DB transaction. This means:
1. The DB updates run → tags are then applied to storage (inside the callback, before TypeORM commits).
2. If a storage call succeeds for file N but then the `dataSource.transaction` machinery fails to commit (e.g., transient DB error at commit time), the DB is rolled back, but the storage tags for file N already point to the target project — the source project data in the DB, the target project tag in storage.
The current `catch` block only runs when the callback itself rejects, not when the post-callback commit fails. If the commit-level error propagates through the `try`, the rollback loop would still run (good), but `rollbackTags` would already be fully populated, so the restoration would execute correctly.
This is a known limitation of non-atomic distributed operations and the best-effort rollback mitigates it. However, the code comment that says "best-effort rollback" only appears in the catch, not at the storage-call site, which may surprise a future maintainer. Consider adding a short inline comment before the `addTags` loop clarifying that these calls are intentionally made inside the callback so that a storage failure rolls back the DB.
How can I resolve this? If you propose a fix, please make it concise.
|
Updated the branch in What changed:
|
Use Case
We need to reorganize legacy datasets (for example consolidating old projects under
moleworks) without client-side copy/re-upload jobs.There is already a single-mission move action in the UI, but it was not consistently available across backend/CLI/API and did not support bulk moves. This PR unifies that path into one server-side move primitive and exposes it in CLI + frontend.
What This PR Changes
POST /missions/movemissionUUIDs: string[]targetProjectUUID: stringnewName(allowed only for single mission)POST /projects/migrate) for full project-to-project consolidation flowsBackend
MoveMissionsDto,MoveMissionsResponseDtoMoveMissionsByBodyGuardwith strict body validation and permission checksmoveMissions(...)implementation with:CLI
mission migrateflow withmission move--mission/-mfor bulk moveget_missionper selector) to prevent partial accidental moves--new-nameusage for multi-mission movesFrontend
POST /mission/movewith bulk payloadMoveCancelno longer triggersonOkpath)Validation
Passed locally:
pytest -q cli/tests/api/test_routes_migrate.py -p no:tests.backend_fixturesnpx -y pnpm@10.27.0 --filter ./backend test -- tests/services/mission-migrate.service.test.ts tests/services/project-migrate.service.test.tsnpx -y pnpm@10.27.0 --filter ./backend buildnpx -y pnpm@10.27.0 --filter ./frontend buildnpx -y prettier --check frontend/src/dialogs/modify-mission-location-dialog.vue frontend/src/pages/missions-explorer-page.vuepython -m py_compile cli/kleinkram/cli/_mission.pyKnown local limitation:
backend/tests/auth/missions/mission-access.test.tsrequires local Postgres; in this environment it fails withECONNREFUSED 127.0.0.1:5432.Reviewer Feedback Addressed
missionUUIDsarray, UUID validation, single-missionnewNamerestriction)ILiketo exact case-insensitiveLOWER(name)=LOWER(:name)