Conversation
Paragon SummaryThis pull request review identified 17 issues across 3 categories in 16 files. The review analyzed code changes, potential bugs, security vulnerabilities, performance issues, and code quality concerns using automated analysis tools. This PR adds version grouping functionality for models and datasets, introducing an asset versions database table, backend service/API endpoints, and frontend UI components (VersionGroupChip and AssetVersionsDrawer) to display and manage grouped asset versions. Key changes:
Confidence score: 3/5
16 files reviewed, 17 comments Severity breakdown: High: 5, Medium: 8, Low: 4 Tip: |
![paragon-review[bot]](https://avatars.githubusercontent.com/in/1544258?s=60&v=4){kind=small}
|
|
||
| async with async_session() as session: | ||
| # Determine next version number | ||
| result = await session.execute( |
There was a problem hiding this comment.
Bug: Race condition in create_version: max(version)+1 without lock or unique constraint
Race condition in create_version: max(version)+1 without lock or unique constraint. Concurrent requests get duplicate version numbers. Add unique constraint on (asset_type, group_name, version).
View Details
Location: api/transformerlab/services/asset_version_service.py (lines 54)
Analysis
Race condition in create_version: max(version)+1 without lock or unique constraint
| What fails | Two concurrent create_version calls for the same group can read the same max(version) and both create the same version number, violating the intended sequential numbering. |
| Result | Two rows exist with the same (asset_type, group_name, version) tuple. Tag resolution and version lookups return unpredictable results. |
| Expected | Each version within a group should have a unique sequential number. The second concurrent request should get version=3. |
| Impact | Data integrity violation. Version resolution becomes non-deterministic with duplicate version numbers in the same group. |
How to reproduce
Send two concurrent POST requests to /asset_versions/versions with the same asset_type and group_name. Both requests read max(version)=1 before either commits, both insert version=2.Patch Details
- __table_args__ = (
- Index("idx_asset_versions_group", "asset_type", "group_name"),
- Index("idx_asset_versions_tag", "asset_type", "group_name", "tag"),
- Index("idx_asset_versions_asset_id", "asset_id"),
- )
+ __table_args__ = (
+ sa.UniqueConstraint("asset_type", "group_name", "version", name="uq_asset_version_group_version"),
+ Index("idx_asset_versions_group", "asset_type", "group_name"),
+ Index("idx_asset_versions_tag", "asset_type", "group_name", "tag"),
+ Index("idx_asset_versions_asset_id", "asset_id"),
+ )AI Fix Prompt
Fix this issue: Race condition in create_version: max(version)+1 without lock or unique constraint. Concurrent requests get duplicate version numbers. Add unique constraint on (asset_type, group_name, version).
Location: api/transformerlab/services/asset_version_service.py (lines 54)
Problem: Two concurrent create_version calls for the same group can read the same max(version) and both create the same version number, violating the intended sequential numbering.
Current behavior: Two rows exist with the same (asset_type, group_name, version) tuple. Tag resolution and version lookups return unpredictable results.
Expected: Each version within a group should have a unique sequential number. The second concurrent request should get version=3.
Steps to reproduce: Send two concurrent POST requests to /asset_versions/versions with the same asset_type and group_name. Both requests read max(version)=1 before either commits, both insert version=2.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
| Endpoints.AssetVersions = { | ||
| ListGroups: (assetType: string) => | ||
| `${API_URL()}asset_versions/groups?asset_type=${assetType}`, | ||
| DeleteGroup: (assetType: string, groupName: string) => |
There was a problem hiding this comment.
Bug: No input sanitization on path-segment parameters groupName/assetType
No input sanitization on path-segment parameters groupName/assetType. Slashes or encoded chars could cause routing issues. Add encodeURIComponent in endpoint builders.
View Details
Location: src/renderer/lib/api-client/endpoints.ts (lines 557)
Analysis
No input sanitization on path-segment parameters groupName/assetType
| What fails | groupName and assetType are interpolated directly into URL path segments without encoding. Group names containing slashes, spaces, or special chars will break routing or hit wrong endpoints. |
| Result | URL path is malformed. The slash in the group name creates extra path segments, causing 404 or routing to unintended endpoints. |
| Expected | All path parameters should be wrapped in encodeURIComponent() to safely handle special characters. |
| Impact | Broken API calls for any group name with special characters. Could also enable path traversal if the backend doesn't validate. |
How to reproduce
Create a version group with name 'my model/v2'. Then call ListVersions('model', 'my model/v2'). The URL becomes `.../versions/model/my model/v2` which routes to the wrong endpoint.Patch Details
- ListVersions: (assetType: string, groupName: string) =>
- `${API_URL()}asset_versions/versions/${assetType}/${groupName}`,
+ ListVersions: (assetType: string, groupName: string) =>
+ `${API_URL()}asset_versions/versions/${encodeURIComponent(assetType)}/${encodeURIComponent(groupName)}`,AI Fix Prompt
Fix this issue: No input sanitization on path-segment parameters groupName/assetType. Slashes or encoded chars could cause routing issues. Add encodeURIComponent in endpoint builders.
Location: src/renderer/lib/api-client/endpoints.ts (lines 557)
Problem: groupName and assetType are interpolated directly into URL path segments without encoding. Group names containing slashes, spaces, or special chars will break routing or hit wrong endpoints.
Current behavior: URL path is malformed. The slash in the group name creates extra path segments, causing 404 or routing to unintended endpoints.
Expected: All path parameters should be wrapped in encodeURIComponent() to safely handle special characters.
Steps to reproduce: Create a version group with name 'my model/v2'. Then call ListVersions('model', 'my model/v2'). The URL becomes `.../versions/model/my model/v2` which routes to the wrong endpoint.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
| connection = op.get_bind() | ||
|
|
||
| # Helper function to check if table exists | ||
| def table_exists(table_name: str) -> bool: |
There was a problem hiding this comment.
Bug: Migration uses sqlite_master, breaking Postgres
Migration uses sqlite_master, breaking Postgres. AGENTS.md says both SQLite and Postgres are supported. Use dialect-agnostic table existence check via sa.inspect.
View Details
Location: api/alembic/versions/a3d2e5f8c901_create_asset_versions_table.py (lines 27)
Analysis
Migration uses sqlite_master, breaking Postgres. AGENTS
| What fails | The Alembic migration hard-codes a SQLite-only system table query (sqlite_master) to check if the table exists, which fails on Postgres deployments. |
| Result | Migration fails with a SQL error on Postgres: relation 'sqlite_master' does not exist. |
| Expected | Migration should run successfully on both SQLite and Postgres using dialect-agnostic introspection. |
| Impact | Blocks deployment on Postgres environments. AGENTS.md explicitly states the app supports both SQLite and Postgres. |
How to reproduce
Run `alembic upgrade head` against a PostgreSQL database. The query `SELECT name FROM sqlite_master WHERE type='table' AND name=:name` will raise an error because sqlite_master doesn't exist in Postgres.Patch Details
- def table_exists(table_name: str) -> bool:
- result = connection.execute(
- sa.text("SELECT name FROM sqlite_master WHERE type='table' AND name=:name"), {"name": table_name}
- )
- return result.fetchone() is not None
+ def table_exists(table_name: str) -> bool:
+ return sa.inspect(connection).has_table(table_name)AI Fix Prompt
Fix this issue: Migration uses sqlite_master, breaking Postgres. AGENTS.md says both SQLite and Postgres are supported. Use dialect-agnostic table existence check via sa.inspect.
Location: api/alembic/versions/a3d2e5f8c901_create_asset_versions_table.py (lines 27)
Problem: The Alembic migration hard-codes a SQLite-only system table query (sqlite_master) to check if the table exists, which fails on Postgres deployments.
Current behavior: Migration fails with a SQL error on Postgres: relation 'sqlite_master' does not exist.
Expected: Migration should run successfully on both SQLite and Postgres using dialect-agnostic introspection.
Steps to reproduce: Run `alembic upgrade head` against a PostgreSQL database. The query `SELECT name FROM sqlite_master WHERE type='table' AND name=:name` will raise an error because sqlite_master doesn't exist in Postgres.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
| """ | ||
| from transformerlab.services import asset_version_service | ||
|
|
||
| try: |
There was a problem hiding this comment.
Bug: Version entry created even when file copy fails
Version entry created even when file copy fails. Dataset/model save catches copy error but proceeds to create_version. Move versioning inside the copy try block.
View Details
Location: api/transformerlab/routers/experiment/jobs.py (lines 1414)
Analysis
**Version entry created even when file copy fails. Dataset/model save catches copy error but proceeds **
| What fails | In save_dataset_to_registry, when storage.copy_dir fails, the exception is caught and only printed (line 1441), then execution continues to create a version entry for a dataset that doesn't actually exist in the registry. |
| Result | A version entry exists in asset_versions pointing to a dataset/model that was never successfully copied to the registry. The API returns 'success'. |
| Expected | If file copy fails, the version entry should NOT be created and the endpoint should return an error. |
| Impact | Ghost version entries referencing non-existent assets. Users see versioned assets that don't exist, causing 404s when trying to use them. |
How to reproduce
1. Trigger a save_dataset_to_registry where the copy fails (e.g., disk full, permissions error). 2. Observe the version entry is still created and 'success' is returned.AI Fix Prompt
Fix this issue: Version entry created even when file copy fails. Dataset/model save catches copy error but proceeds to create_version. Move versioning inside the copy try block.
Location: api/transformerlab/routers/experiment/jobs.py (lines 1414)
Problem: In save_dataset_to_registry, when storage.copy_dir fails, the exception is caught and only printed (line 1441), then execution continues to create a version entry for a dataset that doesn't actually exist in the registry.
Current behavior: A version entry exists in asset_versions pointing to a dataset/model that was never successfully copied to the registry. The API returns 'success'.
Expected: If file copy fails, the version entry should NOT be created and the endpoint should return an error.
Steps to reproduce: 1. Trigger a save_dataset_to_registry where the copy fails (e.g., disk full, permissions error). 2. Observe the version entry is still created and 'success' is returned.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
| version?: number, | ||
| ) => { | ||
| let url = `${API_URL()}asset_versions/resolve/${assetType}/${groupName}`; | ||
| const params: string[] = []; |
There was a problem hiding this comment.
Bug: Endpoint URL params not encoded in Resolve builder
Endpoint URL params not encoded in Resolve builder. Tag/version values with special chars corrupt query string. Use encodeURIComponent on param values.
View Details
Location: src/renderer/lib/api-client/endpoints.ts (lines 577)
Analysis
Endpoint URL params not encoded in Resolve builder. Tag/version values with special chars corrupt qu
| What fails | The Resolve endpoint builder interpolates tag and version values directly into query string parameters without encoding. Tags containing '&' or '=' could corrupt the URL. |
| Result | Query string corruption: extra parameters injected into the URL, potentially causing wrong behavior on the backend. |
| Expected | Use encodeURIComponent() for tag and version values: tag=${encodeURIComponent(tag)}. |
| Impact | Broken API calls for edge-case tag values. Minor query-string injection risk. |
How to reproduce
Call Endpoints.AssetVersions.Resolve('model', 'group', 'tag=evil&admin=true'). The resulting URL has injected query parameters.Patch Details
- if (tag) params.push(`tag=${tag}`);
- if (version !== undefined) params.push(`version=${version}`);
+ if (tag) params.push(`tag=${encodeURIComponent(tag)}`);
+ if (version !== undefined) params.push(`version=${encodeURIComponent(String(version))}`);AI Fix Prompt
Fix this issue: Endpoint URL params not encoded in Resolve builder. Tag/version values with special chars corrupt query string. Use encodeURIComponent on param values.
Location: src/renderer/lib/api-client/endpoints.ts (lines 577)
Problem: The Resolve endpoint builder interpolates tag and version values directly into query string parameters without encoding. Tags containing '&' or '=' could corrupt the URL.
Current behavior: Query string corruption: extra parameters injected into the URL, potentially causing wrong behavior on the backend.
Expected: Use encodeURIComponent() for tag and version values: `tag=${encodeURIComponent(tag)}`.
Steps to reproduce: Call Endpoints.AssetVersions.Resolve('model', 'group', 'tag=evil&admin=true'). The resulting URL has injected query parameters.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
| @@ -33,6 +35,7 @@ export default function DatasetCard({ | |||
| parentMutate, | |||
There was a problem hiding this comment.
Bug: DatasetCard versionGroups prop is untyped (no TypeScript interface)
DatasetCard versionGroups prop is untyped (no TypeScript interface). Props are all untyped in this component. Add explicit prop types for versionGroups.
View Details
Location: src/renderer/components/Data/DatasetCard.tsx (lines 35)
Analysis
DatasetCard versionGroups prop is untyped (no TypeScript interface)
| What fails | The DatasetCard component destructures props without TypeScript interface. The new versionGroups prop defaults to [] but has no type annotation, violating AGENTS.md strict typing rule. |
| Result | No compile-time type checking on versionGroups prop. Passing wrong shape data won't be caught at build time. |
| Expected | AGENTS.md says 'Avoid any. Define interfaces for all props and API responses to ensure type safety.' A Props interface should be defined. |
| Impact | Reduced type safety. Runtime errors possible if wrong data shape is passed to versionGroups. |
How to reproduce
Review DatasetCard.tsx:35-40. All props including the new versionGroups are untyped destructured parameters.AI Fix Prompt
Fix this issue: DatasetCard versionGroups prop is untyped (no TypeScript interface). Props are all untyped in this component. Add explicit prop types for versionGroups.
Location: src/renderer/components/Data/DatasetCard.tsx (lines 35)
Problem: The DatasetCard component destructures props without TypeScript interface. The new versionGroups prop defaults to [] but has no type annotation, violating AGENTS.md strict typing rule.
Current behavior: No compile-time type checking on versionGroups prop. Passing wrong shape data won't be caught at build time.
Expected: AGENTS.md says 'Avoid any. Define interfaces for all props and API responses to ensure type safety.' A Props interface should be defined.
Steps to reproduce: Review DatasetCard.tsx:35-40. All props including the new versionGroups are untyped destructured parameters.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
| description: Mapped[Optional[str]] = mapped_column(String, nullable=True) | ||
| created_at: Mapped[DateTime] = mapped_column(DateTime, server_default=func.now(), nullable=False) | ||
|
|
||
| __table_args__ = ( |
There was a problem hiding this comment.
Bug: Duplicate index definitions between ORM model and migration
Duplicate index definitions between ORM model and migration. Same indexes defined in __table_args__ and migration. Remove from one location to avoid conflicts.
View Details
Location: api/transformerlab/shared/models/models.py (lines 314)
Analysis
Duplicate index definitions between ORM model and migration
| What fails | Index 'idx_asset_versions_group', 'idx_asset_versions_tag', and 'idx_asset_versions_asset_id' are defined in both the Alembic migration and the ORM __table_args__. Additionally, the migration creates extra single-column indexes (ix_asset_versions_asset_type, etc.) that overlap with columns already having index=True in the ORM model. |
| Result | Redundant index definitions that could cause conflicts during autogeneration of future migrations, or errors if Alembic tries to create indexes that already exist. |
| Expected | Indexes should be defined in one authoritative place (preferably the ORM model) and the migration should be auto-generated from it. |
| Impact | Maintenance burden: index changes must be synchronized in two places. Risk of migration errors on fresh databases. |
How to reproduce
Compare indexes in the migration (lines 47-58) with __table_args__ in models.py (lines 314-318) and column-level index=True attributes (lines 301-308).AI Fix Prompt
Fix this issue: Duplicate index definitions between ORM model and migration. Same indexes defined in __table_args__ and migration. Remove from one location to avoid conflicts.
Location: api/transformerlab/shared/models/models.py (lines 314)
Problem: Index 'idx_asset_versions_group', 'idx_asset_versions_tag', and 'idx_asset_versions_asset_id' are defined in both the Alembic migration and the ORM __table_args__. Additionally, the migration creates extra single-column indexes (ix_asset_versions_asset_type, etc.) that overlap with columns already having index=True in the ORM model.
Current behavior: Redundant index definitions that could cause conflicts during autogeneration of future migrations, or errors if Alembic tries to create indexes that already exist.
Expected: Indexes should be defined in one authoritative place (preferably the ORM model) and the migration should be auto-generated from it.
Steps to reproduce: Compare indexes in the migration (lines 47-58) with __table_args__ in models.py (lines 314-318) and column-level index=True attributes (lines 301-308).
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
| model["version_groups"] = group_map[model_id] | ||
| else: | ||
| model["version_groups"] = [] | ||
| except Exception as e: |
There was a problem hiding this comment.
Bug: Silent failure on version group augmentation in model and data list
Silent failure on version group augmentation in model and data list. Bare except catches all errors and only prints. Add proper logging instead of print.
View Details
Location: api/transformerlab/routers/model.py (lines 833)
Analysis
Silent failure on version group augmentation in model and data list
| What fails | Both model_local_list and dataset_list catch all exceptions from version group augmentation with a broad except Exception and only print a warning. Database connection failures, import errors, or schema mismatches are silently swallowed. |
| Result | Version group data silently missing from responses. Only a print statement indicates the failure. No structured logging for monitoring/alerting. |
| Expected | Use the application logger (not print) at WARNING level, and consider whether some errors (like DB connection failures) should propagate. |
| Impact | Debugging difficulty: silent failures in production with no structured logging. Missing version data without any user-visible indication. |
How to reproduce
Break the asset_versions table (e.g., drop it). Call GET /model/list. The warning is printed to stdout but no error is visible to API callers or monitoring.AI Fix Prompt
Fix this issue: Silent failure on version group augmentation in model and data list. Bare except catches all errors and only prints. Add proper logging instead of print.
Location: api/transformerlab/routers/model.py (lines 833)
Problem: Both model_local_list and dataset_list catch all exceptions from version group augmentation with a broad `except Exception` and only print a warning. Database connection failures, import errors, or schema mismatches are silently swallowed.
Current behavior: Version group data silently missing from responses. Only a print statement indicates the failure. No structured logging for monitoring/alerting.
Expected: Use the application logger (not print) at WARNING level, and consider whether some errors (like DB connection failures) should propagate.
Steps to reproduce: Break the asset_versions table (e.g., drop it). Call GET /model/list. The warning is printed to stdout but no error is visible to API callers or monitoring.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
…merlab/transformerlab-app into add/model-dataset-group
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
deep1401
left a comment
deep1401
left a comment
There was a problem hiding this comment.
I fixed a bug in get_artifacts_from_directory and also fixed the down version for alembic.
When trying to click "Save to Registry" on a completed model in the "View Models" modal, it just gets stuck. I waited for 3 mins but there was no error anywhere.
Could you let me know if this is an issue or if I'm testing it wrong somehow?
…merlab/transformerlab-app into add/model-dataset-group
…merlab/transformerlab-app into add/model-dataset-group
This reverts commit 1e208a4.
deep1401
left a comment
deep1401
left a comment
There was a problem hiding this comment.
Keeping aside the model name/group name discrepancy discussion on Discord, I could find 2 bugs. One is that the copy background task that happens seems to create an actual task in the tasks list:
The other bug is that I saved a model and then added another model to the existing group with a different version and it did overwrite my model existing in the models folder:
added both of these changes |
2 participants
Uh oh!
There was an error while loading. Please reload this page.