Fix code correctness: schema trigger, error types, dead code, assertions#10
Fix code correctness: schema trigger, error types, dead code, assertions#10drewburchfield merged 3 commits intomasterfrom
Conversation
- Remove schema trigger that overwrites modified_at with CURRENT_TIMESTAMP, preserving the real file mtime passed by the indexer - Change hub_analyzer error type from ValueError to DatabaseError for consistency with the exception hierarchy - Fix validation docstrings: document that functions raise ValidationError instead of claiming they clamp values - Remove dead empty-content filter in indexer.py (already filtered at lines 119-122) - Remove unused _refresh_all_counts method from hub_analyzer and update test to call _do_refresh directly - Preserve connection_count on upsert (use notes.connection_count instead of resetting to 0) to avoid unnecessary full refreshes - Add ORDER BY chunk_index to graph_builder LIMIT 1 query for deterministic chunk selection - Fix weak test assertion: "pool" or "timeout" always evaluates True, changed to check membership in actual response text
Existing databases retain the mtime-overwriting trigger even after the schema file is updated. Add DROP TRIGGER/FUNCTION IF EXISTS so the init script cleans up on restart. Also fix stale comment.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| -- Migration: remove trigger that overwrites file mtime with DB timestamp | ||
| DROP TRIGGER IF EXISTS trigger_update_notes_modified_at ON notes; | ||
| DROP FUNCTION IF EXISTS update_modified_at(); |
There was a problem hiding this comment.
🟡 Schema migration DROP TRIGGER is unreachable for existing databases
The DROP TRIGGER IF EXISTS migration at src/schema.sql:10-11 is placed inside the init schema file, which is mounted at docker-entrypoint-initdb.d/schema.sql (docker-compose.yml:54). Docker's PostgreSQL entrypoint only executes init scripts on first database creation (when the data volume is empty). For existing databases that already have the trigger, this migration never runs.
This means existing deployments will retain the old trigger_update_notes_modified_at trigger, which overwrites modified_at with CURRENT_TIMESTAMP on every UPDATE — including the _do_refresh batch UPDATE in src/hub_analyzer.py:225-244. This causes all notes' modified_at to be silently reset to the DB timestamp whenever connection counts are refreshed, making the orphaned notes' modified_at display meaningless.
Prompt for agents
The DROP TRIGGER/FUNCTION migration at schema.sql:10-11 is placed in a Docker init script that only runs on fresh database creation. Existing databases that have the trigger will never get it removed.
To fix this, consider one of:
1. Add a separate migration script (e.g., migrations/001_drop_modified_at_trigger.sql) that can be run manually or via a startup migration check in the Python code (e.g., in vector_store.py initialize() method).
2. Add the DROP TRIGGER logic to the Python store initialization code (PostgreSQLVectorStore.initialize), which runs on every server start and can check if the trigger exists before dropping it.
3. Document in the README/CHANGELOG that existing users must manually run: DROP TRIGGER IF EXISTS trigger_update_notes_modified_at ON notes; DROP FUNCTION IF EXISTS update_modified_at();
Was this helpful? React with 👍 or 👎 to provide feedback.
…me migration - Revert connection_count back to = 0 on upsert (preserving counts breaks staleness detection which checks WHERE connection_count = 0; PR #8's inline-await already ensures fresh counts before queries) - Add DROP TRIGGER/FUNCTION to initialize() so existing databases get the migration on server start, not just from schema.sql
| DROP TRIGGER IF EXISTS trigger_update_notes_modified_at ON notes; | ||
| DROP FUNCTION IF EXISTS update_modified_at(); |
There was a problem hiding this comment.
🔴 schema.sql DROP TRIGGER runs before CREATE TABLE, failing on fresh databases
DROP TRIGGER IF EXISTS trigger_update_notes_modified_at ON notes at line 10 executes before CREATE TABLE IF NOT EXISTS notes at line 15. In PostgreSQL, IF EXISTS on DROP TRIGGER only suppresses errors when the trigger doesn't exist — the referenced table must still exist. On a fresh database, the notes table hasn't been created yet, so this statement fails with ERROR: relation "notes" does not exist. Per docker-compose.yml:54, this file is mounted as /docker-entrypoint-initdb.d/schema.sql, meaning it's specifically designed to run on fresh database initialization — the exact scenario where it breaks.
Prompt for agents
The DROP TRIGGER IF EXISTS and DROP FUNCTION IF EXISTS statements at lines 10-11 of schema.sql run before the CREATE TABLE IF NOT EXISTS at line 15. On a fresh database where the notes table does not yet exist, DROP TRIGGER IF EXISTS ... ON notes will fail because PostgreSQL requires the table to exist (IF EXISTS only applies to the trigger, not the table).
The fix is to move these DROP statements after the CREATE TABLE IF NOT EXISTS notes statement. That way, on a fresh DB the table is created first (no trigger exists so the DROPs are no-ops), and on an existing DB the table already exists so the DROPs safely remove the old trigger/function before the CREATE TABLE IF NOT EXISTS is a no-op.
Alternatively, wrap the DROP TRIGGER in a DO block that checks for table existence first, but simply reordering the statements is the simplest fix.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Migration: remove trigger that overwrites file mtime (existing databases) | ||
| await conn.execute( | ||
| "DROP TRIGGER IF EXISTS trigger_update_notes_modified_at ON notes" | ||
| ) | ||
| await conn.execute("DROP FUNCTION IF EXISTS update_modified_at()") |
There was a problem hiding this comment.
🔴 initialize() runs DROP TRIGGER on potentially non-existent table
The migration code at lines 129-132 runs DROP TRIGGER IF EXISTS ... ON notes unconditionally, even when the table_exists check at src/vector_store.py:122-126 determined the notes table does not exist. PostgreSQL's IF EXISTS on DROP TRIGGER only suppresses errors for a missing trigger — the table itself must exist. When the table doesn't exist, this raises an asyncpg.PostgresError which is caught at line 136 and re-raised as VectorStoreError, causing initialize() to fail and preventing the server from starting on a fresh database.
| # Migration: remove trigger that overwrites file mtime (existing databases) | |
| await conn.execute( | |
| "DROP TRIGGER IF EXISTS trigger_update_notes_modified_at ON notes" | |
| ) | |
| await conn.execute("DROP FUNCTION IF EXISTS update_modified_at()") | |
| # Migration: remove trigger that overwrites file mtime (existing databases) | |
| if table_exists: | |
| await conn.execute( | |
| "DROP TRIGGER IF EXISTS trigger_update_notes_modified_at ON notes" | |
| ) | |
| await conn.execute("DROP FUNCTION IF EXISTS update_modified_at()") |
Was this helpful? React with 👍 or 👎 to provide feedback.
…ng table - diagnose_vault.py: compute rel_path before try block to avoid NameError when file open/read fails (Devin PR #9) - schema.sql: move DROP TRIGGER after CREATE TABLE so fresh databases don't fail on non-existent table (Devin PR #10) - vector_store.py: guard DROP TRIGGER migration behind table_exists check to prevent initialize() failure on fresh databases (Devin PR #10)
1 participant
Closes NAS-995
Summary
Test plan