fix: add missing ai_providers columns and fix linux-amd64 build #41

Merged

sarman merged 3 commits from fix/ai-provider-migration-issue into master 2026-04-13 17:00:51 +00:00

Conversation 0 Commits 3 Files Changed 2 +151 -4

sarman commented 2026-04-13 13:23:20 +00:00

Owner

Copy Link

• Add migration 015 to add use_datastore_upload and created_at columns
• Handle column-already-exists errors gracefully
• Update Dockerfile to install linuxdeploy for AppImage bundling
• Add fuse dependency for AppImage support

- Add migration 015 to add use_datastore_upload and created_at columns - Handle column-already-exists errors gracefully - Update Dockerfile to install linuxdeploy for AppImage bundling - Add fuse dependency for AppImage support

sarman added 1 commit 2026-04-13 13:23:21 +00:00

fix: add missing ai_providers columns and fix linux-amd64 build ... 84c69fbea8

- Add migration 015 to add use_datastore_upload and created_at columns
- Handle column-already-exists errors gracefully
- Update Dockerfile to install linuxdeploy for AppImage bundling
- Add fuse dependency for AppImage support

sarman reviewed 2026-04-13 13:26:34 +00:00

sarman left a comment

Author

Owner

Copy Link

🤖 Automated PR Review:\n\n### 1. Summary

This PR addresses two main issues:

• Database migration: Adds two missing columns (use_datastore_upload, created_at) to the ai_providers table via a new migration (015_add_ai_provider_missing_columns), with robust error handling for idempotency.
• Docker build fix: Adds fuse and installs linuxdeploy (required for Tauri 2.x AppImage bundling) to the linux-amd64 Dockerfile.

The changes are generally well-structured, with thoughtful error handling and a comprehensive test. However, there are several critical and high-priority issues that must be addressed before merging.

---

2. Bugs / Errors

🔴 Critical: Incorrect linuxdeploy installation in Dockerfile

+# Install linuxdeploy for AppImage bundling (required for Tauri 2.x)
+    && chmod +x /usr/local/bin/linuxdeploy

➡️ Problem: This line only makes an already-installed linuxdeploy executable — but no curl or wget command downloads it. linuxdeploy is not part of the base image or any prior apt-get install. This will cause the build to fail with:

chmod: cannot access '/usr/local/bin/linuxdeploy': No such file or directory

✅ Fix:

# Install linuxdeploy for AppImage bundling (required for Tauri 2.x)
&& curl -L -o /tmp/linuxdeploy https://github.com/linuxdeploy/linuxdeploy/releases/download/continuous/linuxdeploy-x86_64.AppImage \
&& chmod +x /tmp/linuxdeploy \
&& mv /tmp/linuxdeploy /usr/local/bin/

💡 Bonus: Consider pinning to a specific release (e.g., v1.0.1) instead of continuous for reproducibility.

---

🟠 High: Migration SQL syntax error

"ALTER TABLE ai_providers ADD COLUMN use_datastore_upload INTEGER DEFAULT 0;
 ALTER TABLE ai_providers ADD COLUMN created_at TEXT NOT NULL DEFAULT (datetime('now'));"

➡️ Problem: SQLite does not support multiple ALTER TABLE statements in a single ALTER TABLE command — each ALTER TABLE must be executed separately. The current code passes the entire string to conn.execute_batch(sql), which will fail with:

near ";": syntax error

SQLite only allows one ALTER TABLE statement per command. Docs

✅ Fix:
Split the migration into two separate entries in the migrations array:

(
    "015_add_use_datastore_upload",
    "ALTER TABLE ai_providers ADD COLUMN use_datastore_upload INTEGER DEFAULT 0",
),
(
    "016_add_created_at",
    "ALTER TABLE ai_providers ADD COLUMN created_at TEXT NOT NULL DEFAULT (datetime('now'))",
),

Or, if you want to keep a single migration name (e.g., 015_add_ai_provider_missing_columns), use two execute calls in the migration runner — but the current migrations loop expects one SQL string per migration.

⚠️ Note: If you keep the current format, execute_batch will fail. You’d need to split the SQL manually or use execute twice per migration — but that breaks the current abstraction.

---

🟠 High: Test uses i64 for INTEGER column, but SQLite stores booleans as 0/1

let (name, use_datastore_upload, created_at): (String, i64, String) = ...

➡️ Problem: use_datastore_upload is declared as INTEGER (no PRIMARY KEY, no CHECK, no BOOLEAN type). SQLite has no BOOLEAN type — it stores 0/1 as integers. Using i64 is technically correct, but semantically misleading.

✅ Fix: Either:

• Use bool (rusqlite supports bool mapping to 0/1):

  let (name, use_datastore_upload, created_at): (String, bool, String) = ...
  assert!(!use_datastore_upload);
  

• Or keep i64, but add a comment: // SQLite INTEGER used for boolean (0/1)

Also, consider adding a CHECK (use_datastore_upload IN (0, 1)) constraint in the migration.

---

🟡 Medium: Test doesn’t verify idempotency (skipping on duplicate columns)

The migration has logic to handle duplicate column name, but the test only verifies the success path. The error-handling branch is untested.

✅ Fix: Add a test case:

#[test]
fn test_idempotent_add_missing_columns() {
    let conn = Connection::open_in_memory().unwrap();

    // Create table with both columns already present
    conn.execute_batch(
        "CREATE TABLE IF NOT EXISTS ai_providers (
            id TEXT PRIMARY KEY,
            name TEXT NOT NULL UNIQUE,
            provider_type TEXT NOT NULL,
            api_url TEXT NOT NULL,
            encrypted_api_key TEXT NOT NULL,
            model TEXT NOT NULL,
            max_tokens INTEGER,
            temperature REAL,
            custom_endpoint_path TEXT,
            custom_auth_header TEXT,
            custom_auth_prefix TEXT,
            api_format TEXT,
            user_id TEXT,
            use_datastore_upload INTEGER DEFAULT 0,
            created_at TEXT NOT NULL DEFAULT (datetime('now')),
            updated_at TEXT NOT NULL DEFAULT (datetime('now'))
        );",
    ).unwrap();

    // Should not fail
    run_migrations(&conn).unwrap();
}

---

3. Security Issues

🟢 No direct security issues found, but:

• use_datastore_upload is an INTEGER — ensure it’s used as a boolean (0/1) in application logic. If misused (e.g., as a count), it could lead to logic bugs (e.g., uploading unintended data). Recommend adding:

  ALTER TABLE ai_providers ADD COLUMN use_datastore_upload INTEGER DEFAULT 0 CHECK (use_datastore_upload IN (0, 1));
  

• created_at is TEXT NOT NULL DEFAULT (datetime('now')). Ensure all consumers parse it consistently (e.g., ISO 8601). SQLite’s datetime('now') returns YYYY-MM-DD HH:MM:SS, which is safe and standard.

---

4. Best Practices

✅ Good practices present:

• Idempotent migrations with error handling for duplicate columns.
• Comprehensive in-memory SQLite test verifying schema and default values.
• Clear commit message and changelog-style comments.

🟡 Improvements recommended:

1. Migration naming convention
   Current: 015_add_ai_provider_missing_columns
   Better: 015_add_missing_columns_to_ai_providers
   → Avoids ambiguity (e.g., "missing" could refer to many columns).

2. Use TIMESTAMP type for created_at?
   TEXT with ISO 8601 is fine, but consider INTEGER (Unix epoch) for easier sorting/comparisons.
   → If keeping TEXT, ensure all app code uses chrono::DateTime<Utc> or equivalent.

3. Add migration version to schema
   Consider adding a schema_version table to track applied migrations explicitly (though rusqlite/migrations pattern likely handles this internally).

4. Dockerfile: Use multi-stage for final image
   The current Dockerfile installs Rust toolchain and build deps but doesn’t show the final image. Ensure linuxdeploy is not included in the final runtime image (only needed for bundling).
   → Add comment: # linuxdeploy only needed during bundling, not runtime

5. Add updated_at to migration?
   The table definition in tests already has updated_at, but the migration SQL doesn’t mention it. Ensure it’s not accidentally dropped or duplicated.

---

Final Recommendation

Do not merge as-is. Fix the following before merging:

1. ✅ Add linuxdeploy download step (critical).
2. ✅ Split ALTER TABLE statements (critical).
3. ✅ Add idempotency test (high priority to prevent regressions).

After fixes, this is a solid, production-ready PR.\n\n---\nthis is an automated review from Ollama

🤖 Automated PR Review:\n\n### **1. Summary** This PR addresses two main issues: - **Database migration**: Adds two missing columns (`use_datastore_upload`, `created_at`) to the `ai_providers` table via a new migration (`015_add_ai_provider_missing_columns`), with robust error handling for idempotency. - **Docker build fix**: Adds `fuse` and installs `linuxdeploy` (required for Tauri 2.x AppImage bundling) to the `linux-amd64` Dockerfile. The changes are generally well-structured, with thoughtful error handling and a comprehensive test. However, there are several **critical and high-priority issues** that must be addressed before merging. --- ### **2. Bugs / Errors** #### 🔴 **Critical: Incorrect `linuxdeploy` installation in Dockerfile** ```diff +# Install linuxdeploy for AppImage bundling (required for Tauri 2.x) + && chmod +x /usr/local/bin/linuxdeploy ``` ➡️ **Problem**: This line *only* makes an *already-installed* `linuxdeploy` executable — but **no `curl` or `wget` command downloads it**. `linuxdeploy` is not part of the base image or any prior `apt-get install`. This will cause the build to fail with: ``` chmod: cannot access '/usr/local/bin/linuxdeploy': No such file or directory ``` ✅ **Fix**: ```dockerfile # Install linuxdeploy for AppImage bundling (required for Tauri 2.x) && curl -L -o /tmp/linuxdeploy https://github.com/linuxdeploy/linuxdeploy/releases/download/continuous/linuxdeploy-x86_64.AppImage \ && chmod +x /tmp/linuxdeploy \ && mv /tmp/linuxdeploy /usr/local/bin/ ``` > 💡 **Bonus**: Consider pinning to a specific release (e.g., `v1.0.1`) instead of `continuous` for reproducibility. --- #### 🟠 **High: Migration SQL syntax error** ```rust "ALTER TABLE ai_providers ADD COLUMN use_datastore_upload INTEGER DEFAULT 0; ALTER TABLE ai_providers ADD COLUMN created_at TEXT NOT NULL DEFAULT (datetime('now'));" ``` ➡️ **Problem**: SQLite **does not support multiple `ALTER TABLE` statements in a single `ALTER TABLE` command** — each `ALTER TABLE` must be executed *separately*. The current code passes the entire string to `conn.execute_batch(sql)`, which will fail with: ``` near ";": syntax error ``` > SQLite only allows one `ALTER TABLE` statement per command. [Docs](https://www.sqlite.org/lang_altertable.html) ✅ **Fix**: Split the migration into two separate entries in the `migrations` array: ```rust ( "015_add_use_datastore_upload", "ALTER TABLE ai_providers ADD COLUMN use_datastore_upload INTEGER DEFAULT 0", ), ( "016_add_created_at", "ALTER TABLE ai_providers ADD COLUMN created_at TEXT NOT NULL DEFAULT (datetime('now'))", ), ``` Or, if you want to keep a single migration name (e.g., `015_add_ai_provider_missing_columns`), use two `execute` calls in the migration runner — but the current `migrations` loop expects one SQL string per migration. > ⚠️ **Note**: If you keep the current format, `execute_batch` will fail. You’d need to split the SQL manually or use `execute` twice per migration — but that breaks the current abstraction. --- #### 🟠 **High: Test uses `i64` for `INTEGER` column, but SQLite stores booleans as `0/1`** ```rust let (name, use_datastore_upload, created_at): (String, i64, String) = ... ``` ➡️ **Problem**: `use_datastore_upload` is declared as `INTEGER` (no `PRIMARY KEY`, no `CHECK`, no `BOOLEAN` type). SQLite has no `BOOLEAN` type — it stores `0`/`1` as integers. Using `i64` is *technically correct*, but semantically misleading. ✅ **Fix**: Either: - Use `bool` (rusqlite supports `bool` mapping to `0/1`): ```rust let (name, use_datastore_upload, created_at): (String, bool, String) = ... assert!(!use_datastore_upload); ``` - Or keep `i64`, but add a comment: `// SQLite INTEGER used for boolean (0/1)` Also, consider adding a `CHECK (use_datastore_upload IN (0, 1))` constraint in the migration. --- #### 🟡 **Medium: Test doesn’t verify idempotency (skipping on duplicate columns)** The migration has logic to handle `duplicate column name`, but the test only verifies the *success* path. The error-handling branch is untested. ✅ **Fix**: Add a test case: ```rust #[test] fn test_idempotent_add_missing_columns() { let conn = Connection::open_in_memory().unwrap(); // Create table with both columns already present conn.execute_batch( "CREATE TABLE IF NOT EXISTS ai_providers ( id TEXT PRIMARY KEY, name TEXT NOT NULL UNIQUE, provider_type TEXT NOT NULL, api_url TEXT NOT NULL, encrypted_api_key TEXT NOT NULL, model TEXT NOT NULL, max_tokens INTEGER, temperature REAL, custom_endpoint_path TEXT, custom_auth_header TEXT, custom_auth_prefix TEXT, api_format TEXT, user_id TEXT, use_datastore_upload INTEGER DEFAULT 0, created_at TEXT NOT NULL DEFAULT (datetime('now')), updated_at TEXT NOT NULL DEFAULT (datetime('now')) );", ).unwrap(); // Should not fail run_migrations(&conn).unwrap(); } ``` --- ### **3. Security Issues** #### 🟢 **No direct security issues found**, but: - `use_datastore_upload` is an `INTEGER` — ensure it’s used as a boolean (0/1) in application logic. If misused (e.g., as a count), it could lead to logic bugs (e.g., uploading unintended data). Recommend adding: ```sql ALTER TABLE ai_providers ADD COLUMN use_datastore_upload INTEGER DEFAULT 0 CHECK (use_datastore_upload IN (0, 1)); ``` - `created_at` is `TEXT NOT NULL DEFAULT (datetime('now'))`. Ensure all consumers parse it consistently (e.g., ISO 8601). SQLite’s `datetime('now')` returns `YYYY-MM-DD HH:MM:SS`, which is safe and standard. --- ### **4. Best Practices** #### ✅ **Good practices present**: - Idempotent migrations with error handling for duplicate columns. - Comprehensive in-memory SQLite test verifying schema and default values. - Clear commit message and changelog-style comments. #### 🟡 **Improvements recommended**: 1. **Migration naming convention** Current: `015_add_ai_provider_missing_columns` Better: `015_add_missing_columns_to_ai_providers` → Avoids ambiguity (e.g., "missing" could refer to many columns). 2. **Use `TIMESTAMP` type for `created_at`?** `TEXT` with ISO 8601 is fine, but consider `INTEGER` (Unix epoch) for easier sorting/comparisons. → If keeping `TEXT`, ensure all app code uses `chrono::DateTime<Utc>` or equivalent. 3. **Add migration version to schema** Consider adding a `schema_version` table to track applied migrations explicitly (though rusqlite/migrations pattern likely handles this internally). 4. **Dockerfile: Use multi-stage for final image** The current Dockerfile installs Rust toolchain and build deps but doesn’t show the final image. Ensure `linuxdeploy` is *not* included in the final runtime image (only needed for bundling). → Add comment: `# linuxdeploy only needed during bundling, not runtime` 5. **Add `updated_at` to migration?** The table definition in tests already has `updated_at`, but the migration SQL doesn’t mention it. Ensure it’s not accidentally dropped or duplicated. --- ### **Final Recommendation** **Do not merge as-is.** Fix the following **before merging**: 1. ✅ **Add `linuxdeploy` download step** (critical). 2. ✅ **Split `ALTER TABLE` statements** (critical). 3. ✅ **Add idempotency test** (high priority to prevent regressions). After fixes, this is a solid, production-ready PR.\n\n---\n*this is an automated review from Ollama*

sarman added 1 commit 2026-04-13 13:38:55 +00:00

fix: address AI review findings ... 2d7aac8413

- Add -L flag to curl for linuxdeploy redirects
- Split migration 015 into 015_add_use_datastore_upload and 016_add_created_at
- Use separate execute calls for ALTER TABLE statements
- Add idempotency test for migration 015
- Use bool type for use_datastore_upload instead of i64

sarman reviewed 2026-04-13 13:42:26 +00:00

sarman left a comment

Author

Owner

Copy Link

🤖 Automated PR Review:\n\n### 1) Summary
This PR addresses two issues:

• Database migrations: Adds missing use_datastore_upload and created_at columns to the ai_providers table, with idempotent handling for partial/multiple migration runs.
• Linux/amd64 Docker build: Installs fuse and linuxdeploy to support AppImage bundling for Tauri 2.x.

The migration logic now gracefully handles column-already-exists errors, and includes comprehensive unit tests for idempotency and data integrity.

---

2) Bugs / Errors

❌ Critical: linuxdeploy installation is incomplete and broken

In the Dockerfile, linuxdeploy is referenced but never actually downloaded or installed:

# Install linuxdeploy for AppImage bundling (required for Tauri 2.x)
# Download linuxdeploy from the official repository and extract AppImage
    && chmod +x /usr/local/bin/linuxdeploy

➡️ Problem: chmod is executed on a non-existent file (/usr/local/bin/linuxdeploy), which will cause the Docker build to fail with chmod: cannot access '/usr/local/bin/linuxdeploy': No such file or directory.

✅ Fix (recommended approach):

# Install linuxdeploy for AppImage bundling (required for Tauri 2.x)
RUN curl -Ls "https://github.com/$(curl -s https://api.github.com/repos/$(curl -s https://api.github.com/repos/tauri-apps/tauri/releases/latest | jq -r .body | grep -oP 'linuxdeploy.*AppImage' | head -1 | sed 's/.*\/linuxdeploy-\([^\/]*\)\.AppImage.*/tauri-apps\/linuxdeploy/releases\/download\/v0.0.0\/linuxdeploy-\1.AppImage/g' | sed 's/v0.0.0/$(curl -s https://api.github.com/repos/tauri-apps/linuxdeploy/releases/latest | jq -r .tag_name)'/g)" \
    -o /usr/local/bin/linuxdeploy \
    && chmod +x /usr/local/bin/linuxdeploy

But that’s overly complex and fragile. A more robust, maintainable solution:

# Install linuxdeploy (v1.0.1 as of writing)
ENV LINUXDEPLOY_VERSION=1.0.1
RUN curl -Ls "https://github.com/tauri-apps/linuxdeploy/releases/download/${LINUXDEPLOY_VERSION}/linuxdeploy-${LINUXDEPLOY_VERSION}-x86_64.AppImage" \
    -o /tmp/linuxdeploy.AppImage \
    && chmod +x /tmp/linuxdeploy.AppImage \
    && /tmp/linuxdeploy.AppImage --appimage-extract \
    && mv squashfs-root/usr/bin/linuxdeploy /usr/local/bin/ \
    && rm -rf squashfs-root /tmp/linuxdeploy.AppImage

🔔 Note: linuxdeploy is distributed as an AppImage that must be extracted (or run with --appimage-extract). Direct chmod on the .AppImage is insufficient — the binary is inside.

---

⚠️ Migration SQL: created_at column uses datetime('now') but type is TEXT — could cause issues

"ALTER TABLE ai_providers ADD COLUMN created_at TEXT NOT NULL DEFAULT (datetime('now'))"

➡️ Problem:

• datetime('now') returns a string like "2024-06-15 14:30:22" — acceptable for TEXT, but:
• SQLite will treat it as a literal string, not a timestamp — no automatic timezone handling, no datetime() function compatibility unless explicitly cast.
• If future code expects ISO 8601 or UTC, this is fine — but if other parts of the app use CURRENT_TIMESTAMP (which is TEXT in SQLite), it’s still inconsistent.

✅ Fix: Explicitly document the format and ensure consistency across the codebase.
Better yet: use TIMESTAMP (SQLite treats it as TEXT anyway) but add a comment:

"ALTER TABLE ai_providers ADD COLUMN created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%d %H:%M:%S', 'now'))"

→ strftime gives more control and avoids ambiguous datetime() behavior.

Also: consider using created_at TEXT NOT NULL DEFAULT (datetime('now', 'utc')) if UTC is required.

---

⚠️ Migration error handling is overly broad

if err_str.contains("duplicate column name")
    || err_str.contains("has no column named")

➡️ Problem:

• "has no column named" is a runtime query error, not a DDL error — it may appear in unrelated contexts (e.g., SELECT on a malformed table).
• "duplicate column name" is correct for ALTER TABLE ADD COLUMN, but "has no column named" is misleading here — SQLite returns "duplicate column name" for duplicate ADD COLUMN, but "has no column named" is for invalid column references in SELECT, UPDATE, etc.

✅ Fix: Use precise error codes or strings. SQLite error codes:

• SQLITE_ERROR (1) with "duplicate column name" → err_str.contains("duplicate column name") ✅
• But "has no column named" is not expected in ALTER TABLE ADD COLUMN, so it should be treated as a real error.

→ Only check for "duplicate column name":

if err_str.contains("duplicate column name") {
    tracing::info!(...);
} else {
    return Err(e.into());
}

---

3) Security Issues

✅ No critical security issues found.

• No hardcoded secrets or credentials.
• Migrations use parameterized execute()/execute_batch() — safe.
• linuxdeploy installation (if fixed) uses HTTPS from GitHub releases — secure if checksums or signatures are verified (but that’s out of scope for this PR; assume trusted source for now).

---

4) Best Practices

✅ Strong migration idempotency testing

The added unit tests (test_add_missing_columns_to_existing_table, test_idempotent_add_missing_columns) are excellent:

• In-memory DB simulates real-world schema evolution.
• Verifies both fresh and already-migrated states.
• Tests default values and data integrity.

✅ Clear separation of execute vs execute_batch

Correctly uses execute() for single ALTER TABLE (SQLite limitation) and execute_batch() for multi-statement migrations (e.g., FTS5). This avoids subtle bugs.

⚠️ Migration names should be deterministic and versioned

"015_add_use_datastore_upload",
"016_add_created_at",

➡️ Risk: If multiple PRs add migrations, naming collisions can occur (e.g., two 015_... files).
✅ Best practice: Use timestamps or UUIDs for uniqueness:

"20240615120000_add_use_datastore_upload",
"20240615120001_add_created_at",

Or adopt a migration framework (e.g., migrate-rs) that handles ordering.

⚠️ Missing rollback plan

SQLite ALTER TABLE has limited rollback (only via transaction, but no DROP COLUMN in older SQLite versions).
➡️ If migration 015/016 fails partway, the table may be left in an inconsistent state (e.g., use_datastore_upload added but created_at not).
✅ Recommendation:

• Wrap all migrations in a transaction:

let tx = conn.transaction()?;
for (name, sql) in migrations {
    // ...
    tx.execute_batch(sql)?;
}
tx.commit()?;

• Or at least document that migrations are non-atomic and require manual recovery.

⚠️ Dockerfile comment is outdated

# Rebuild when: Rust toolchain version changes, webkit2gtk/gtk major version changes,
# Node.js major version changes, OpenSSL major version changes (used via OPENSSL_STATIC=1),
# or Tauri CLI version changes that affect bundler system deps, or linuxdeploy is needed.

➡️ linuxdeploy is now always needed for Tauri 2.x AppImage builds — update comment to reflect that.

---

✅ Final Recommendations

Issue	Severity	Suggested Fix
linuxdeploy not installed	🔴 Critical	Add download + extraction logic
"has no column named" error check too broad	🟠 Medium	Remove that condition
datetime('now') vs strftime(..., 'utc')	🟡 Low	Standardize timestamp format
Migration naming (numeric)	🟡 Medium	Use timestamps or UUIDs
No transactional migration safety	🟡 Medium	Wrap in conn.transaction()
Dockerfile comment outdated	🟢 Low	Update to reflect linuxdeploy requirement

Let me know if you'd like a corrected Dockerfile snippet or migration code patch!\n\n---\nthis is an automated review from Ollama

🤖 Automated PR Review:\n\n### 1) Summary This PR addresses two issues: - **Database migrations**: Adds missing `use_datastore_upload` and `created_at` columns to the `ai_providers` table, with idempotent handling for partial/multiple migration runs. - **Linux/amd64 Docker build**: Installs `fuse` and `linuxdeploy` to support AppImage bundling for Tauri 2.x. The migration logic now gracefully handles column-already-exists errors, and includes comprehensive unit tests for idempotency and data integrity. --- ### 2) Bugs / Errors #### ❌ **Critical: `linuxdeploy` installation is incomplete and broken** In the Dockerfile, `linuxdeploy` is referenced but **never actually downloaded or installed**: ```dockerfile # Install linuxdeploy for AppImage bundling (required for Tauri 2.x) # Download linuxdeploy from the official repository and extract AppImage && chmod +x /usr/local/bin/linuxdeploy ``` ➡️ **Problem**: `chmod` is executed on a non-existent file (`/usr/local/bin/linuxdeploy`), which will cause the Docker build to fail with `chmod: cannot access '/usr/local/bin/linuxdeploy': No such file or directory`. ✅ **Fix** (recommended approach): ```dockerfile # Install linuxdeploy for AppImage bundling (required for Tauri 2.x) RUN curl -Ls "https://github.com/$(curl -s https://api.github.com/repos/$(curl -s https://api.github.com/repos/tauri-apps/tauri/releases/latest | jq -r .body | grep -oP 'linuxdeploy.*AppImage' | head -1 | sed 's/.*\/linuxdeploy-\([^\/]*\)\.AppImage.*/tauri-apps\/linuxdeploy/releases\/download\/v0.0.0\/linuxdeploy-\1.AppImage/g' | sed 's/v0.0.0/$(curl -s https://api.github.com/repos/tauri-apps/linuxdeploy/releases/latest | jq -r .tag_name)'/g)" \ -o /usr/local/bin/linuxdeploy \ && chmod +x /usr/local/bin/linuxdeploy ``` But that’s overly complex and fragile. A more robust, maintainable solution: ```dockerfile # Install linuxdeploy (v1.0.1 as of writing) ENV LINUXDEPLOY_VERSION=1.0.1 RUN curl -Ls "https://github.com/tauri-apps/linuxdeploy/releases/download/${LINUXDEPLOY_VERSION}/linuxdeploy-${LINUXDEPLOY_VERSION}-x86_64.AppImage" \ -o /tmp/linuxdeploy.AppImage \ && chmod +x /tmp/linuxdeploy.AppImage \ && /tmp/linuxdeploy.AppImage --appimage-extract \ && mv squashfs-root/usr/bin/linuxdeploy /usr/local/bin/ \ && rm -rf squashfs-root /tmp/linuxdeploy.AppImage ``` > 🔔 **Note**: `linuxdeploy` is distributed as an AppImage that must be extracted (or run with `--appimage-extract`). Direct `chmod` on the `.AppImage` is insufficient — the binary is inside. --- #### ⚠️ **Migration SQL: `created_at` column uses `datetime('now')` but type is `TEXT` — could cause issues** ```rust "ALTER TABLE ai_providers ADD COLUMN created_at TEXT NOT NULL DEFAULT (datetime('now'))" ``` ➡️ **Problem**: - `datetime('now')` returns a **string** like `"2024-06-15 14:30:22"` — acceptable for `TEXT`, but: - SQLite will treat it as a *literal string*, not a timestamp — no automatic timezone handling, no `datetime()` function compatibility unless explicitly cast. - If future code expects ISO 8601 or UTC, this is fine — but if other parts of the app use `CURRENT_TIMESTAMP` (which is `TEXT` in SQLite), it’s still inconsistent. ✅ **Fix**: Explicitly document the format and ensure consistency across the codebase. Better yet: use `TIMESTAMP` (SQLite treats it as `TEXT` anyway) but add a comment: ```rust "ALTER TABLE ai_providers ADD COLUMN created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%d %H:%M:%S', 'now'))" ``` → `strftime` gives more control and avoids ambiguous `datetime()` behavior. Also: consider using `created_at TEXT NOT NULL DEFAULT (datetime('now', 'utc'))` if UTC is required. --- #### ⚠️ **Migration error handling is overly broad** ```rust if err_str.contains("duplicate column name") || err_str.contains("has no column named") ``` ➡️ **Problem**: - `"has no column named"` is a **runtime query error**, not a DDL error — it may appear in unrelated contexts (e.g., `SELECT` on a malformed table). - `"duplicate column name"` is correct for `ALTER TABLE ADD COLUMN`, but `"has no column named"` is misleading here — SQLite returns `"duplicate column name"` for duplicate `ADD COLUMN`, but `"has no column named"` is for invalid column references in `SELECT`, `UPDATE`, etc. ✅ **Fix**: Use precise error codes or strings. SQLite error codes: - `SQLITE_ERROR` (1) with `"duplicate column name"` → `err_str.contains("duplicate column name")` ✅ - But `"has no column named"` is **not** expected in `ALTER TABLE ADD COLUMN`, so it should be treated as a real error. → **Only check for `"duplicate column name"`**: ```rust if err_str.contains("duplicate column name") { tracing::info!(...); } else { return Err(e.into()); } ``` --- ### 3) Security Issues #### ✅ No critical security issues found. - No hardcoded secrets or credentials. - Migrations use parameterized `execute()`/`execute_batch()` — safe. - `linuxdeploy` installation (if fixed) uses HTTPS from GitHub releases — secure *if* checksums or signatures are verified (but that’s out of scope for this PR; assume trusted source for now). --- ### 4) Best Practices #### ✅ **Strong migration idempotency testing** The added unit tests (`test_add_missing_columns_to_existing_table`, `test_idempotent_add_missing_columns`) are excellent: - In-memory DB simulates real-world schema evolution. - Verifies both fresh and already-migrated states. - Tests default values and data integrity. #### ✅ **Clear separation of `execute` vs `execute_batch`** Correctly uses `execute()` for single `ALTER TABLE` (SQLite limitation) and `execute_batch()` for multi-statement migrations (e.g., FTS5). This avoids subtle bugs. #### ⚠️ **Migration names should be deterministic and versioned** ```rust "015_add_use_datastore_upload", "016_add_created_at", ``` ➡️ **Risk**: If multiple PRs add migrations, naming collisions can occur (e.g., two `015_...` files). ✅ **Best practice**: Use timestamps or UUIDs for uniqueness: ```rust "20240615120000_add_use_datastore_upload", "20240615120001_add_created_at", ``` Or adopt a migration framework (e.g., `migrate-rs`) that handles ordering. #### ⚠️ **Missing rollback plan** SQLite `ALTER TABLE` has limited rollback (only via transaction, but no `DROP COLUMN` in older SQLite versions). ➡️ If migration 015/016 fails *partway*, the table may be left in an inconsistent state (e.g., `use_datastore_upload` added but `created_at` not). ✅ **Recommendation**: - Wrap all migrations in a transaction: ```rust let tx = conn.transaction()?; for (name, sql) in migrations { // ... tx.execute_batch(sql)?; } tx.commit()?; ``` - Or at least document that migrations are non-atomic and require manual recovery. #### ⚠️ **Dockerfile comment is outdated** ```dockerfile # Rebuild when: Rust toolchain version changes, webkit2gtk/gtk major version changes, # Node.js major version changes, OpenSSL major version changes (used via OPENSSL_STATIC=1), # or Tauri CLI version changes that affect bundler system deps, or linuxdeploy is needed. ``` ➡️ `linuxdeploy` is now *always* needed for Tauri 2.x AppImage builds — update comment to reflect that. --- ### ✅ Final Recommendations | Issue | Severity | Suggested Fix | |-------|----------|---------------| | `linuxdeploy` not installed | 🔴 Critical | Add download + extraction logic | | `"has no column named"` error check too broad | 🟠 Medium | Remove that condition | | `datetime('now')` vs `strftime(..., 'utc')` | 🟡 Low | Standardize timestamp format | | Migration naming (numeric) | 🟡 Medium | Use timestamps or UUIDs | | No transactional migration safety | 🟡 Medium | Wrap in `conn.transaction()` | | Dockerfile comment outdated | 🟢 Low | Update to reflect `linuxdeploy` requirement | Let me know if you'd like a corrected Dockerfile snippet or migration code patch!\n\n---\n*this is an automated review from Ollama*

sarman added 1 commit 2026-04-13 16:55:07 +00:00

fix: address critical AI review issues ... 8e1d43da43

- Fix linuxdeploy AppImage extraction using --appimage-extract
- Remove 'has no column named' from duplicate column error handling
- Use strftime instead of datetime for created_at default format

sarman reviewed 2026-04-13 16:58:31 +00:00

sarman left a comment

Author

Owner

Copy Link

🤖 Automated PR Review:\n\n### 1. Summary

This PR addresses two key issues:

• Database migration: Adds missing columns (use_datastore_upload, created_at) to the ai_providers table with idempotent handling for re-runs.
• Linux/amd64 Docker build: Installs linuxdeploy (required for Tauri 2.x AppImage bundling) and updates documentation comments.

The changes are well-structured, include thorough tests, and follow common Rust/SQLite patterns. However, there are critical correctness issues in the migration logic and a security concern in the Dockerfile.

---

2. Bugs / Errors

🔴 Critical: Incorrect error handling for ALTER TABLE migrations

In migrations.rs, the logic for handling 015_add_use_datastore_upload and 016_add_created_at is flawed:

} else if name.ends_with("_add_use_datastore_upload")
    || name.ends_with("_add_created_at")
{
    if let Err(e) = conn.execute(sql, []) {
        let err_str = e.to_string();
        if err_str.contains("duplicate column name") {
            tracing::info!("Column may already exist, skipping migration {name}: {e}");
        } else {
            return Err(e.into());
        }
    }
}

Problem:

• conn.execute(sql, []) is used for ALTER TABLE, but SQLite’s execute() only supports one statement per call, and ALTER TABLE is a single statement — ✅ correct in principle.
• However, the error string matching is fragile: e.to_string() may not always contain "duplicate column name" verbatim. SQLite error messages can vary by version, locale, or build (e.g., "duplicate column name: use_datastore_upload" vs "duplicate column name"), and rusqlite wraps errors (e.g., rusqlite::Error::SqliteFailure), so .to_string() may include extra context.

Worse: If the error message format changes (e.g., due to SQLite upgrade), the migration could silently fail to add the column on re-run, or panic on unexpected error.

✅ Suggested fix: Use rusqlite::ErrorCode to check for the exact error code:

use rusqlite::ErrorCode;

// ...
} else if name.ends_with("_add_use_datastore_upload")
    || name.ends_with("_add_created_at")
{
    match conn.execute(sql, []) {
        Ok(_) => {}
        Err(rusqlite::Error::SqliteFailure(err, _)) if err.extended_code == ErrorCode::ConstraintViolation as i32 => {
            // SQLite returns SQLITE_CONSTRAINT (error code 19) for duplicate column
            tracing::info!("Column already exists, skipping migration {name}: {err}");
        }
        Err(e) => return Err(e.into()),
    }
}

💡 Note: SQLite returns SQLITE_CONSTRAINT (not SQLITE_ERROR) for duplicate columns. The exact extended error code is ErrorCode::ConstraintViolation. Check rusqlite docs for accuracy.

🟡 Minor: Inconsistent use of execute() vs execute_batch()

The refactored logic now uses execute() for ALTER TABLE and execute_batch() for others — good. But the comment says:

// Use execute for ALTER TABLE (SQLite only allows one statement per command)

This is true, but the previous code used execute_batch() for all non-FTS migrations, which would have failed for ALTER TABLE (since ALTER TABLE ...; ALTER TABLE ... is invalid). So the fix is correct only because each migration SQL string is a single ALTER TABLE statement.

✅ But: Ensure future migrations don’t accidentally add multi-statement SQL (e.g., ALTER TABLE ...; CREATE INDEX ...) — consider adding a linter or comment warning.

---

3. Security Issues

🔴 Dockerfile: Insecure linuxdeploy download and extraction

# Install linuxdeploy for AppImage bundling (required for Tauri 2.x)
# Download linuxdeploy AppImage and extract to get the binary
    && chmod +x /tmp/linuxdeploy.AppImage \
    && /tmp/linuxdeploy.AppImage --appimage-extract \
    && mv squashfs-root/usr/bin/linuxdeploy /usr/local/bin/ \
    && rm -rf /tmp/linuxdeploy.AppImage squashfs-root

Problem:

• No checksum verification of the downloaded linuxdeploy.AppImage.
• No version pinning — the download URL is missing entirely in the diff! (This is a critical omission — the PR likely assumes a hardcoded URL, but it’s not shown.)

The full Dockerfile likely has:

RUN curl -fsSL https://github.com/.../linuxdeploy-x86_64.AppImage -o /tmp/linuxdeploy.AppImage

But without a checksum, this is vulnerable to:

• Supply-chain attacks (MITM, compromised GitHub release)
• Unintended behavior if linuxdeploy changes behavior between versions

✅ Suggested fix:

1. Pin the version (e.g., linuxdeploy 1.0.1).
2. Verify SHA256 checksum:

RUN curl -fsSL "https://github.com/$(...)/linuxdeploy-x86_64.AppImage" -o /tmp/linuxdeploy.AppImage \
    && echo "abc123...  /tmp/linuxdeploy.AppImage" | sha256sum -c - \
    && chmod +x /tmp/linuxdeploy.AppImage \
    && /tmp/linuxdeploy.AppImage --appimage-extract \
    && mv squashfs-root/usr/bin/linuxdeploy /usr/local/bin/ \
    && rm -rf /tmp/linuxdeploy.AppImage squashfs-root

3. Prefer official package if available (e.g., via apt or snap), though AppImage is common for linuxdeploy.

🔒 Best practice: Store checksums in a trusted file (e.g., linuxdeploy.sha256) in the repo, and use curl with --compressed and --fail flags.

---

4. Best Practices

✅ Good: Comprehensive tests

• Tests cover both new table and pre-existing table scenarios.
• Tests verify column presence and default values.
• Idempotency (test_idempotent_add_missing_columns) is well-tested.

✅ Good: Graceful handling of FTS5

• Skips FTS5 migrations if not compiled in — appropriate.

⚠️ Improve: Migration naming & ordering

• Migrations are named 015_add_use_datastore_upload, 016_add_created_at.
• But: created_at should ideally be added before use_datastore_upload if there are dependencies (though ALTER TABLE is independent here).
• Better: Use timestamp-based naming (e.g., 20240615120000_add_use_datastore_upload) to avoid ordering issues across branches.

⚠️ Improve: Default value consistency

• use_datastore_upload is INTEGER DEFAULT 0, but the test uses bool:

  let (name, use_datastore_upload, created_at): (String, bool, String) = ...
  

  SQLite stores INTEGER for bool, and rusqlite maps 0/1 to bool automatically — ✅ works, but clarify in comments or use BOOLEAN (SQLite treats BOOLEAN as INTEGER anyway).

✅ Good: Documentation updates

• Dockerfile comment updated to reflect linuxdeploy need.

---

Final Recommendations

Issue	Severity	Action
Error handling for duplicate columns	🔴 Critical	Use rusqlite::ErrorCode instead of string matching
Missing checksum/version in Dockerfile	🔴 Critical	Pin version + verify SHA256
Migration naming	🟡 Medium	Use timestamp-based names
Test coverage	✅ Excellent	Keep as-is

Before merging, ensure:

1. The full Dockerfile URL + checksum are included (or referenced in a follow-up).
2. The migration error handling uses rusqlite::ErrorCode::ConstraintViolation.
3. A changelog entry is added (if applicable).

Let me know if you’d like a patch for the corrected migration logic!\n\n---\nthis is an automated review from Ollama

🤖 Automated PR Review:\n\n### **1. Summary** This PR addresses two key issues: - **Database migration**: Adds missing columns (`use_datastore_upload`, `created_at`) to the `ai_providers` table with idempotent handling for re-runs. - **Linux/amd64 Docker build**: Installs `linuxdeploy` (required for Tauri 2.x AppImage bundling) and updates documentation comments. The changes are well-structured, include thorough tests, and follow common Rust/SQLite patterns. However, there are **critical correctness issues** in the migration logic and **a security concern** in the Dockerfile. --- ### **2. Bugs / Errors** #### 🔴 **Critical: Incorrect error handling for `ALTER TABLE` migrations** In `migrations.rs`, the logic for handling `015_add_use_datastore_upload` and `016_add_created_at` is **flawed**: ```rust } else if name.ends_with("_add_use_datastore_upload") || name.ends_with("_add_created_at") { if let Err(e) = conn.execute(sql, []) { let err_str = e.to_string(); if err_str.contains("duplicate column name") { tracing::info!("Column may already exist, skipping migration {name}: {e}"); } else { return Err(e.into()); } } } ``` **Problem**: - `conn.execute(sql, [])` is used for `ALTER TABLE`, but **SQLite’s `execute()` only supports one statement per call**, and `ALTER TABLE` is a single statement — ✅ correct *in principle*. - However, **the error string matching is fragile**: `e.to_string()` may not always contain `"duplicate column name"` verbatim. SQLite error messages can vary by version, locale, or build (e.g., `"duplicate column name: use_datastore_upload"` vs `"duplicate column name"`), and rusqlite wraps errors (e.g., `rusqlite::Error::SqliteFailure`), so `.to_string()` may include extra context. **Worse**: If the error message format changes (e.g., due to SQLite upgrade), the migration could **silently fail to add the column** on re-run, or **panic** on unexpected error. ✅ **Suggested fix**: Use `rusqlite::ErrorCode` to check for the exact error code: ```rust use rusqlite::ErrorCode; // ... } else if name.ends_with("_add_use_datastore_upload") || name.ends_with("_add_created_at") { match conn.execute(sql, []) { Ok(_) => {} Err(rusqlite::Error::SqliteFailure(err, _)) if err.extended_code == ErrorCode::ConstraintViolation as i32 => { // SQLite returns SQLITE_CONSTRAINT (error code 19) for duplicate column tracing::info!("Column already exists, skipping migration {name}: {err}"); } Err(e) => return Err(e.into()), } } ``` > 💡 **Note**: SQLite returns `SQLITE_CONSTRAINT` (not `SQLITE_ERROR`) for duplicate columns. The exact extended error code is `ErrorCode::ConstraintViolation`. Check [rusqlite docs](https://docs.rs/rusqlite/latest/rusqlite/enum.ErrorCode.html) for accuracy. #### 🟡 **Minor: Inconsistent use of `execute()` vs `execute_batch()`** The refactored logic now uses `execute()` for `ALTER TABLE` and `execute_batch()` for others — good. But the comment says: > `// Use execute for ALTER TABLE (SQLite only allows one statement per command)` This is **true**, but the previous code used `execute_batch()` for all non-FTS migrations, which would have failed for `ALTER TABLE` (since `ALTER TABLE ...; ALTER TABLE ...` is invalid). So the fix is correct *only because* each migration SQL string is a single `ALTER TABLE` statement. ✅ **But**: Ensure future migrations don’t accidentally add multi-statement SQL (e.g., `ALTER TABLE ...; CREATE INDEX ...`) — consider adding a linter or comment warning. --- ### **3. Security Issues** #### 🔴 **Dockerfile: Insecure `linuxdeploy` download and extraction** ```dockerfile # Install linuxdeploy for AppImage bundling (required for Tauri 2.x) # Download linuxdeploy AppImage and extract to get the binary && chmod +x /tmp/linuxdeploy.AppImage \ && /tmp/linuxdeploy.AppImage --appimage-extract \ && mv squashfs-root/usr/bin/linuxdeploy /usr/local/bin/ \ && rm -rf /tmp/linuxdeploy.AppImage squashfs-root ``` **Problem**: - **No checksum verification** of the downloaded `linuxdeploy.AppImage`. - **No version pinning** — the download URL is missing entirely in the diff! (This is a **critical omission** — the PR likely assumes a hardcoded URL, but it’s not shown.) The full Dockerfile likely has: ```dockerfile RUN curl -fsSL https://github.com/.../linuxdeploy-x86_64.AppImage -o /tmp/linuxdeploy.AppImage ``` But **without a checksum**, this is vulnerable to: - **Supply-chain attacks** (MITM, compromised GitHub release) - **Unintended behavior** if `linuxdeploy` changes behavior between versions ✅ **Suggested fix**: 1. **Pin the version** (e.g., `linuxdeploy 1.0.1`). 2. **Verify SHA256 checksum**: ```dockerfile RUN curl -fsSL "https://github.com/$(...)/linuxdeploy-x86_64.AppImage" -o /tmp/linuxdeploy.AppImage \ && echo "abc123... /tmp/linuxdeploy.AppImage" | sha256sum -c - \ && chmod +x /tmp/linuxdeploy.AppImage \ && /tmp/linuxdeploy.AppImage --appimage-extract \ && mv squashfs-root/usr/bin/linuxdeploy /usr/local/bin/ \ && rm -rf /tmp/linuxdeploy.AppImage squashfs-root ``` 3. **Prefer official package** if available (e.g., via `apt` or `snap`), though AppImage is common for `linuxdeploy`. > 🔒 **Best practice**: Store checksums in a trusted file (e.g., `linuxdeploy.sha256`) in the repo, and use `curl` with `--compressed` and `--fail` flags. --- ### **4. Best Practices** #### ✅ **Good: Comprehensive tests** - Tests cover both **new table** and **pre-existing table** scenarios. - Tests verify column presence and default values. - Idempotency (`test_idempotent_add_missing_columns`) is well-tested. #### ✅ **Good: Graceful handling of FTS5** - Skips FTS5 migrations if not compiled in — appropriate. #### ⚠️ **Improve: Migration naming & ordering** - Migrations are named `015_add_use_datastore_upload`, `016_add_created_at`. - **But**: `created_at` should ideally be added *before* `use_datastore_upload` if there are dependencies (though `ALTER TABLE` is independent here). - **Better**: Use **timestamp-based naming** (e.g., `20240615120000_add_use_datastore_upload`) to avoid ordering issues across branches. #### ⚠️ **Improve: Default value consistency** - `use_datastore_upload` is `INTEGER DEFAULT 0`, but the test uses `bool`: ```rust let (name, use_datastore_upload, created_at): (String, bool, String) = ... ``` SQLite stores `INTEGER` for `bool`, and rusqlite maps `0/1` to `bool` automatically — ✅ works, but clarify in comments or use `BOOLEAN` (SQLite treats `BOOLEAN` as `INTEGER` anyway). #### ✅ **Good: Documentation updates** - Dockerfile comment updated to reflect `linuxdeploy` need. --- ### **Final Recommendations** | Issue | Severity | Action | |-------|----------|--------| | Error handling for duplicate columns | 🔴 **Critical** | Use `rusqlite::ErrorCode` instead of string matching | | Missing checksum/version in Dockerfile | 🔴 **Critical** | Pin version + verify SHA256 | | Migration naming | 🟡 **Medium** | Use timestamp-based names | | Test coverage | ✅ **Excellent** | Keep as-is | **Before merging**, ensure: 1. The full Dockerfile URL + checksum are included (or referenced in a follow-up). 2. The migration error handling uses `rusqlite::ErrorCode::ConstraintViolation`. 3. A changelog entry is added (if applicable). Let me know if you’d like a patch for the corrected migration logic!\n\n---\n*this is an automated review from Ollama*

sarman merged commit 05f87a7bff into master 2026-04-13 17:00:51 +00:00

sarman referenced this issue from a commit 2026-04-13 17:00:52 +00:00

Merge pull request 'fix: add missing ai_providers columns and fix linux-amd64 build' (#41) from fix/ai-provider-migration-issue into master

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

Compat/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

No Label

Compat/Breaking

Kind/Bug

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

1 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sarman/tftsr-devops_investigation#41

No description provided.