refactor(store): maintain updated_at with a trigger, not by hand (RIG-3495)

go/internal/store/migrations/0001_init.sql

@@ -961,3 +961,75 @@ BEGIN
         $f$, t);
     END LOOP;
 END $$;
+
+-- ── updated_at maintenance (RIG-3495) ───────────────────────────────────────
+-- created_at + updated_at is the schema convention for every table from here
+-- on, and updated_at is maintained by THIS trigger — never by a hand-written
+-- `updated_at = now()` in a query file.
+--
+-- Hand-maintenance is the failure mode, not a hypothetical one: secrets.updated_at
+-- rotted exactly that way. The column was declared here, read by DeclaredSecrets
+-- and surfaced on SecretDeclaration.UpdatedAt, but no write statement in
+-- queries/secrets.sql ever set it — so the value could only ever equal
+-- created_at, and every caller reading it was reading a lie. A per-statement
+-- convention that is invisible at the point a NEW write statement is added
+-- cannot survive; a trigger is enforced by the table, so a write path added
+-- later inherits it without anyone remembering.
+--
+-- The rule for a new table: give it created_at + updated_at with the usual
+-- DEFAULT now(), add its name to updated_at_tables below, and set updated_at
+-- NOWHERE else. There must be exactly one mechanism.
+--
+-- NOT covered, deliberately: issues.forge_updated_at and
+-- forge_repo_subscriptions.swept_updated_at. Those are forge-supplied
+-- watermarks — the remote's mutation time and the sweep high-water mark — not
+-- this row's local mutation time, and their writers set them explicitly
+-- (issues.sql's upsert even guards on the incoming value going forward). They
+-- are differently named precisely so this trigger cannot reach them.
+--
+-- search_path safety: the function is SECURITY INVOKER (the default, and what
+-- we want — a trigger doing NEW.updated_at = now() needs no elevated rights),
+-- but it still runs with whatever search_path the CALLING session has set, so
+-- an unqualified name in the body could be resolved against a schema the caller
+-- controls. Pinning search_path on the function makes the body's resolution
+-- independent of the caller. pg_catalog alone is enough and is the right pin
+-- here: the body resolves exactly one name, now(), which lives in pg_catalog —
+-- and this migration is applied into a per-test isolation schema as often as
+-- into public (internal/pgshare hands each test its own schema via search_path),
+-- so naming `public` would pin to a schema that is NOT the one holding these
+-- tables. NEW/OLD are parser-level, not search_path-resolved.
+CREATE FUNCTION set_updated_at() RETURNS TRIGGER
+    LANGUAGE plpgsql
+    SET search_path = pg_catalog
+AS $$
+BEGIN
+    NEW.updated_at = now();
+    RETURN NEW;
+END $$;
+
+-- One BEFORE UPDATE ... FOR EACH ROW trigger per table carrying updated_at.
+-- BEFORE (not AFTER) because the trigger mutates the row being written, and
+-- UPDATE only (not INSERT) because the column's DEFAULT now() already stamps an
+-- inserted row — a BEFORE INSERT here would merely re-derive the same value
+-- while destroying the ability to insert a row with a deliberate updated_at.
+-- An INSERT ... ON CONFLICT DO UPDATE fires this on the conflict path, which is
+-- what makes every upsert in queries/ keep the column live for free. Done in a
+-- DO loop for the same reason the RLS block above is: so the identical trigger
+-- is never copy-pasted per table, and adding a table is one array entry.
+DO $$
+DECLARE
+    t text;
+    updated_at_tables text[] := ARRAY[
+        'secrets',
+        'agent_placements',
+        'agent_config_bundle',
+        'model_registry',
+        'forge_repo_subscriptions'
+    ];
+BEGIN
+    FOREACH t IN ARRAY updated_at_tables LOOP
+        EXECUTE format(
+            'CREATE TRIGGER set_updated_at BEFORE UPDATE ON %I
+                 FOR EACH ROW EXECUTE FUNCTION set_updated_at()', t);
+    END LOOP;
+END $$;

go/internal/store/queries/agent_config.sql

@@ -8,7 +8,7 @@
 INSERT INTO agent_config_bundle (singleton, version, bundle)
 VALUES (TRUE, $1, $2)
 ON CONFLICT (singleton)
-DO UPDATE SET version = EXCLUDED.version, bundle = EXCLUDED.bundle, updated_at = now();
+DO UPDATE SET version = EXCLUDED.version, bundle = EXCLUDED.bundle;
 
 -- name: CurrentAgentConfig :one
 SELECT version, bundle FROM agent_config_bundle WHERE singleton = TRUE;

go/internal/store/queries/agent_placements.sql

@@ -8,8 +8,7 @@ INSERT INTO agent_placements (agent_account_id, runner_id, container_name)
 VALUES ($1, $2, $3)
 ON CONFLICT (agent_account_id) DO UPDATE
    SET runner_id      = EXCLUDED.runner_id,
-       container_name = EXCLUDED.container_name,
-       updated_at     = now();
+       container_name = EXCLUDED.container_name;
 
 -- name: AgentForContainer :one
 SELECT agent_account_id FROM agent_placements WHERE container_name = $1;

go/internal/store/queries/forge_cursors.sql

@@ -13,7 +13,7 @@ WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3;
 
 -- name: StoreForgeRepoWatermark :execrows
 UPDATE forge_repo_subscriptions
-   SET swept_updated_at = $4, list_etag = $5, updated_at = now()
+   SET swept_updated_at = $4, list_etag = $5
  WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3;
 
 -- name: EnsureForgeRepoSubscription :exec
@@ -40,5 +40,5 @@ ORDER BY repo ASC;
 
 -- name: SetForgeRepoSubscriptionEnabled :execrows
 UPDATE forge_repo_subscriptions
-   SET enabled = $4, updated_at = now()
+   SET enabled = $4
  WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3;

go/internal/store/queries/model_registry.sql

@@ -25,7 +25,7 @@ RETURNING version;
 -- ErrVersionConflict.
 -- name: UpdateModelRegistry :one
 UPDATE model_registry
-   SET registry = $1, version = version + 1, updated_at = now()
+   SET registry = $1, version = version + 1
  WHERE singleton = TRUE AND version = $2
 RETURNING version;
 

go/internal/store/updated_at_pgtest_test.go

@@ -0,0 +1,189 @@
+//go:build pgtest
+
+package store
+
+// The set_updated_at trigger convention (RIG-3495), proven against real
+// Postgres. updated_at is maintained by ONE mechanism — the BEFORE UPDATE
+// trigger 0001_init.sql installs on every table carrying the column — and no
+// query file sets it by hand. These tests are the enforcement of that: they
+// fail if the trigger is missing, which is exactly what happens if someone
+// re-adds a table without an updated_at_tables entry, or drops the block.
+//
+// Three properties, each a distinct failure mode:
+//
+//  1. A plain UPDATE advances updated_at and leaves created_at alone. The
+//     created_at half is not padding: it is what catches a BEFORE INSERT OR
+//     UPDATE trigger, which would look correct on every update assertion while
+//     silently making created_at meaningless.
+//  2. The INSERT ... ON CONFLICT DO UPDATE conflict path fires it. This is the
+//     case most likely to be wrong, since the trigger is declared on UPDATE and
+//     an upsert reads as an insert. Driven through the real store method
+//     (RecordAgentPlacement), not raw SQL, so the covered thing is the path
+//     production takes.
+//  3. secrets.updated_at is live — the rot this change fixes.
+//
+// now() is TRANSACTION time in Postgres, so two writes inside one transaction
+// observe the identical value. Every write below is its own statement on the
+// pool (its own implicit transaction), which is what makes a strict > correct.
+// A time.Sleep would only mask a wrong assertion; there is none here.
+//
+// context.Background is the test root (the pgtest-suite convention, sibling
+// forge_cursors_pgtest_test.go).
+
+import (
+	"context"
+	"testing"
+	"time"
+)
+
+// TestUpdatedAtTriggerAdvancesOnUpdate proves property 1 on
+// forge_repo_subscriptions: SetForgeRepoSubscriptionEnabled is a plain UPDATE
+// that no longer sets updated_at itself, so an advanced value can only have
+// come from the trigger — and created_at must not move with it.
+func TestUpdatedAtTriggerAdvancesOnUpdate(t *testing.T) {
+	ctx := context.Background()
+	s := newTestStore(t)
+
+	sub := ForgeRepoSubscription{
+		Provider: ForgeProviderGitHub,
+		Host:     "github.com",
+		Repo:     "a/b",
+		Enabled:  true,
+	}
+	if err := s.EnsureForgeRepoSubscription(ctx, sub); err != nil {
+		t.Fatalf("EnsureForgeRepoSubscription: %v", err)
+	}
+	createdBefore, updatedBefore := repoSubStamps(t, s, sub)
+	if !updatedBefore.Equal(createdBefore) {
+		t.Fatalf("on insert updated_at %v != created_at %v; both default now() in one statement",
+			updatedBefore, createdBefore)
+	}
+
+	// Separate statement => separate transaction => a strictly later now().
+	if err := s.SetForgeRepoSubscriptionEnabled(ctx, sub.Provider, sub.Host, sub.Repo, false); err != nil {
+		t.Fatalf("SetForgeRepoSubscriptionEnabled: %v", err)
+	}
+	createdAfter, updatedAfter := repoSubStamps(t, s, sub)
+
+	if !updatedAfter.After(updatedBefore) {
+		t.Errorf("updated_at did not advance on UPDATE: before=%v after=%v (set_updated_at trigger missing?)",
+			updatedBefore, updatedAfter)
+	}
+	if !createdAfter.Equal(createdBefore) {
+		t.Errorf("created_at moved on UPDATE: before=%v after=%v (trigger fires on INSERT too?)",
+			createdBefore, createdAfter)
+	}
+}
+
+// TestUpdatedAtTriggerFiresOnUpsertConflict proves property 2: an
+// INSERT ... ON CONFLICT DO UPDATE takes the UPDATE path on conflict and so
+// fires the BEFORE UPDATE trigger. RecordAgentPlacement is that upsert, and its
+// query no longer carries a hand-written updated_at.
+func TestUpdatedAtTriggerFiresOnUpsertConflict(t *testing.T) {
+	ctx := context.Background()
+	s := newTestStore(t)
+	owner := mustUser(t, s, "placement-owner")
+	agent := mustAgent(t, s, owner.ID, "placed")
+
+	if err := s.RecordAgentPlacement(ctx, agent.ID, "runner-1", "compass-agent-1"); err != nil {
+		t.Fatalf("RecordAgentPlacement (insert): %v", err)
+	}
+	createdBefore, updatedBefore := placementStamps(t, s, agent.ID)
+
+	// Same agent_account_id => the ON CONFLICT DO UPDATE arm.
+	if err := s.RecordAgentPlacement(ctx, agent.ID, "runner-2", "compass-agent-1"); err != nil {
+		t.Fatalf("RecordAgentPlacement (conflict): %v", err)
+	}
+	createdAfter, updatedAfter := placementStamps(t, s, agent.ID)
+
+	if !updatedAfter.After(updatedBefore) {
+		t.Errorf("updated_at did not advance on the upsert conflict path: before=%v after=%v",
+			updatedBefore, updatedAfter)
+	}
+	if !createdAfter.Equal(createdBefore) {
+		t.Errorf("created_at moved on the upsert conflict path: before=%v after=%v",
+			createdBefore, createdAfter)
+	}
+}
+
+// TestSecretsUpdatedAtIsLive proves property 3 — the specific rot RIG-3495
+// fixes. secrets.updated_at is declared, read by DeclaredSecrets, and surfaced
+// on SecretDeclaration.UpdatedAt, but queries/secrets.sql has only an INSERT and
+// a DELETE: no write path ever set the column, so its value could never differ
+// from created_at and every reader was reading a lie.
+//
+// The store therefore still has no update method for a secret, and this test
+// does NOT invent one. It asserts what the store's own surface can show (a
+// freshly declared row has updated_at == created_at), and then drives a bare
+// UPDATE on the table to prove the trigger is ARMED on secrets — so the column
+// becomes correct for free the moment a re-declare/rotate path is added, rather
+// than needing whoever adds it to remember.
+func TestSecretsUpdatedAtIsLive(t *testing.T) {
+	ctx := context.Background()
+	s := newTestStore(t)
+	actor := mustUser(t, s, "secrets-owner")
+
+	if err := s.DeclareSecret(ctx, actor.ID, "DATABASE_URL", SecretDeliveryEnv, SecretKindGeneric, "", ""); err != nil {
+		t.Fatalf("DeclareSecret: %v", err)
+	}
+	createdBefore, updatedBefore := secretStamps(t, s, "DATABASE_URL")
+	if !updatedBefore.Equal(createdBefore) {
+		t.Fatalf("on declare updated_at %v != created_at %v; both default now() in one statement",
+			updatedBefore, createdBefore)
+	}
+
+	// A mutation of the row's own data, in its own transaction. The statement
+	// deliberately does NOT mention updated_at: only the trigger can move it.
+	if _, err := s.pool.Exec(ctx,
+		`UPDATE secrets SET delivery = $2 WHERE name = $1`,
+		"DATABASE_URL", int32(SecretDeliveryFile),
+	); err != nil {
+		t.Fatalf("update secret delivery: %v", err)
+	}
+	createdAfter, updatedAfter := secretStamps(t, s, "DATABASE_URL")
+
+	if !updatedAfter.After(updatedBefore) {
+		t.Errorf("secrets.updated_at did not advance on UPDATE: before=%v after=%v (the rot is not fixed)",
+			updatedBefore, updatedAfter)
+	}
+	if !createdAfter.Equal(createdBefore) {
+		t.Errorf("secrets.created_at moved on UPDATE: before=%v after=%v",
+			createdBefore, createdAfter)
+	}
+}
+
+// repoSubStamps reads a forge repo subscription's (created_at, updated_at).
+func repoSubStamps(t *testing.T, s *Store, sub ForgeRepoSubscription) (created, updated time.Time) {
+	t.Helper()
+	if err := s.pool.QueryRow(context.Background(),
+		`SELECT created_at, updated_at FROM forge_repo_subscriptions
+		  WHERE forge_provider = $1 AND forge_host = $2 AND repo = $3`,
+		int16(sub.Provider), sub.Host, sub.Repo,
+	).Scan(&created, &updated); err != nil {
+		t.Fatalf("read forge_repo_subscriptions stamps: %v", err)
+	}
+	return created, updated
+}
+
+// placementStamps reads an agent placement's (created_at, updated_at).
+func placementStamps(t *testing.T, s *Store, agent AccountID) (created, updated time.Time) {
+	t.Helper()
+	if err := s.pool.QueryRow(context.Background(),
+		`SELECT created_at, updated_at FROM agent_placements WHERE agent_account_id = $1`,
+		string(agent),
+	).Scan(&created, &updated); err != nil {
+		t.Fatalf("read agent_placements stamps: %v", err)
+	}
+	return created, updated
+}
+
+// secretStamps reads a declared secret's (created_at, updated_at).
+func secretStamps(t *testing.T, s *Store, name string) (created, updated time.Time) {
+	t.Helper()
+	if err := s.pool.QueryRow(context.Background(),
+		`SELECT created_at, updated_at FROM secrets WHERE name = $1`, name,
+	).Scan(&created, &updated); err != nil {
+		t.Fatalf("read secrets stamps: %v", err)
+	}
+	return created, updated
+}