sledtools/pika branch #115

pika-git-delete-legacy-pr-1

pika-git: delete legacy compatibility surface

Target branch: master

Merge Commit: 2f6e793760ed2f28ff30d1beffcc9ffe50320e2f

branch: merged tutorial: ready ci: success

Open CI Details

Continuous Integration

CI: success

Compact status on the review page, with full logs on the CI page.

Open CI Details

Latest run #142 success

2 passed

head 6be8889cbde6153056b229754957f2549916fdc2 · queued 2026-03-26 21:02:40 · 2 lane(s)

queued 6s · ran 29s

check-notifications · success check-agent-contracts · success

Summary

This branch removes the legacy "pika-news" compatibility surface from the pika-git service. The project was previously renamed from pika-news to pika-git, and temporary shims were left in place (a /news URL alias, dual localStorage key fallbacks, Option-wrapped PR fields on inbox items, a legacy database migration ledger name, legacy state directory migration logic, and legacy secret key names). This branch drops all of that: it adds a new SQL migration to remove orphaned PR-related tables, renames the migrations ledger from _pika_news_migrations to pika_git_migrations with a safe transition path, simplifies the InboxItem struct to non-optional branch-only fields, strips the /news route alias and localStorage compat helpers, removes the Nix pre-start database/directory migration script, and re-keys the SOPS secrets under their canonical pika_git names.

Tutorial Steps

Add SQL migration to drop legacy PR-related tables

Intent: Remove database tables that belonged to the old pull-request-centric review surface (chat_messages, chat_sessions, artifact_user_states, artifact_versions, inbox_dismissals, inbox, generated_artifacts, pull_requests, poll_markers). These tables are no longer referenced anywhere in the codebase.

Affected files: crates/pika-git/migrations/0025_drop_legacy_pr_surface.sql, crates/pika-git/src/storage.rs

Evidence

@@ -0,0 +1,13 @@
+PRAGMA foreign_keys = OFF;
+
+DROP TABLE IF EXISTS chat_messages;
+DROP TABLE IF EXISTS chat_sessions;
+DROP TABLE IF EXISTS artifact_user_states;
+DROP TABLE IF EXISTS artifact_versions;
+DROP TABLE IF EXISTS inbox_dismissals;
+DROP TABLE IF EXISTS inbox;
+DROP TABLE IF EXISTS generated_artifacts;
+DROP TABLE IF EXISTS pull_requests;
+DROP TABLE IF EXISTS poll_markers;
+
+PRAGMA foreign_keys = ON;

@@ -634,6 +668,11 @@ fn migrations() -> Vec<Migration> {
+        Migration {
+            version: 25,
+            name: "0025_drop_legacy_pr_surface",
+            sql: include_str!("../migrations/0025_drop_legacy_pr_surface.sql"),
+        },

A new migration (0025_drop_legacy_pr_surface.sql) is added that unconditionally drops nine tables left over from the legacy PR review workflow. PRAGMA foreign_keys is toggled off/on around the drops to avoid FK constraint errors during teardown.

The migration is registered in the migrations() vec as version 25 in storage.rs:668-672.

Rename the migrations ledger table from _pika_news_migrations to _pika_git_migrations

Intent: Complete the pika-news → pika-git rename at the database metadata level. The migration ledger table that tracks which SQL migrations have been applied is renamed, with a safe three-way transition: rename if only the legacy table exists, merge-and-drop if both exist, or create fresh if neither exists.

Affected files: crates/pika-git/src/storage.rs

Evidence

@@ -401,23 +398,29 @@
+const MIGRATIONS_TABLE: &str = "_pika_git_migrations";
+const LEGACY_MIGRATIONS_TABLE: &str = "_pika_news_migrations";

@@ -452,6 +459,33 @@
+fn ensure_migrations_table(conn: &Connection) -> anyhow::Result<()> {
+    let has_current = table_exists(conn, MIGRATIONS_TABLE)?;
+    let has_legacy = table_exists(conn, LEGACY_MIGRATIONS_TABLE)?;
+
+    if has_current && has_legacy {
+        let merge_sql = format!(
+            "INSERT OR IGNORE INTO {MIGRATIONS_TABLE}(version, name, applied_at)
+             SELECT version, name, applied_at FROM {LEGACY_MIGRATIONS_TABLE}"
+        );
+        conn.execute(&merge_sql, [])
+            .context("merge legacy migration ledger into current ledger")?;
+        let drop_legacy_sql = format!("DROP TABLE {LEGACY_MIGRATIONS_TABLE}");
+        conn.execute_batch(&drop_legacy_sql)
+            .context("drop legacy migration ledger after merge")?;
+        return Ok(());
+    }
+
+    if !has_current && has_legacy {
+        let rename_sql =
+            format!("ALTER TABLE {LEGACY_MIGRATIONS_TABLE} RENAME TO {MIGRATIONS_TABLE}");
+        conn.execute_batch(&rename_sql)
+            .context("rename legacy migration ledger to current name")?;
+    }
+
+    Ok(());
+}

Two constants are introduced at storage.rs:400-401:

const MIGRATIONS_TABLE: &str = "_pika_git_migrations";
const LEGACY_MIGRATIONS_TABLE: &str = "_pika_news_migrations";

A new function ensure_migrations_table (storage.rs:459-491) runs before the main migration loop and handles three cases:

Both tables exist — merge rows from legacy into current with INSERT OR IGNORE, then drop the legacy table.
Only legacy exists — ALTER TABLE … RENAME TO the current name.
Neither exists — no-op; the subsequent CREATE TABLE IF NOT EXISTS in run_migrations will create it.

All SQL strings in run_migrations that previously hard-coded _pika_news_migrations now use the MIGRATIONS_TABLE constant via format!.

Simplify InboxItem struct — remove Option wrappers and PR fields

Intent: Since the legacy PR surface is gone, InboxItem no longer needs to represent both branch reviews and PR reviews. Fields that were Option-wrapped for dual-mode compatibility (branch_id, branch_name) become non-optional, and PR-only fields (pr_id, pr_number, url) are deleted entirely.

Affected files: crates/pika-git/src/storage.rs, crates/pika-git/src/inbox_store.rs

Evidence

@@ -358,13 +358,10 @@
 pub struct InboxItem {
     pub review_id: i64,
-    pub branch_id: Option<i64>,
-    pub pr_id: Option<i64>,
+    pub branch_id: i64,
     pub repo: String,
-    pub branch_name: Option<String>,
-    pub pr_number: Option<i64>,
+    pub branch_name: String,
     pub title: String,
-    pub url: Option<String>,

@@ -111,13 +111,10 @@
                     Ok(InboxItem {
                         review_id: row.get(0)?,
-                        branch_id: Some(row.get(0)?),
-                        pr_id: None,
+                        branch_id: row.get(0)?,
                         repo: row.get(1)?,
-                        branch_name: Some(row.get(2)?),
-                        pr_number: None,
+                        branch_name: row.get(2)?,
                         title: row.get(3)?,
-                        url: None,

The InboxItem struct (storage.rs:358-367) is slimmed down:

Removed	Changed
`pr_id: Option<i64>`	`branch_id: Option<i64>` → `branch_id: i64`
`pr_number: Option<i64>`	`branch_name: Option<String>` → `branch_name: String`
`url: Option<String>`

The query-mapping code in inbox_store.rs:111-120 is updated in tandem — it no longer wraps values in Some(…) or sets PR fields to None.

Remove the /news route alias from the web server

Intent: The /news path was kept as a temporary redirect alias while the rename from pika-news to pika-git rolled out. That transition period is over, so the alias is removed.

Affected files: crates/pika-git/src/web.rs

Evidence

@@ -784,8 +784,6 @@
     let app = Router::new()
         .route("/", get(|| async { Redirect::permanent("/git") }))
         .nest("/git", git_routes.clone())
-        // Keep `/news` alive as a compatibility alias during the rename rollout.
-        .nest("/news", git_routes)

In web.rs:784-787, the .nest("/news", git_routes) line and its explanatory comment are deleted. The router now only mounts routes under /git.

Remove localStorage legacy-key migration helpers from the frontend

Intent: The base HTML template contained JavaScript helpers that read/wrote values under both pika_git_ and pika_news_ localStorage prefixes so users wouldn't lose their auth tokens during the rename. With the transition complete these shims are removed.

Affected files: crates/pika-git/templates/base.html

Evidence

@@ -97,25 +97,16 @@
-        function oldAuthKey(key) {
-          if (!key.startsWith('pika_git_')) return key;
-          return 'pika_news_' + key.slice('pika_git_'.length);
-        }
-
         function getAuthValue(key) {
-          const current = authStorage.getItem(key);
-          if (current !== null) return current;
-          return authStorage.getItem(oldAuthKey(key));
+          return authStorage.getItem(key);
         }

@@ -272,10 +263,7 @@
         window.addEventListener('storage', function (event) {
-          if (
-            !event.key
-            || (!event.key.startsWith('pika_git_') && !event.key.startsWith('pika_news_'))
-          ) return;
+          if (!event.key || !event.key.startsWith('pika_git_')) return;

Three functions in base.html are simplified:

oldAuthKey() — deleted entirely.
getAuthValue(key) — no longer falls back to the legacy prefix; just calls authStorage.getItem(key).
setAuthValue(key, value) / removeAuthValue(key) — no longer clean up the legacy-prefixed key.

The cross-tab storage event listener now only reacts to keys starting with pika_git_, dropping the pika_news_ check.

Simplify inbox template — remove PR vs. branch rendering fork

Intent: The inbox item rendering previously branched on whether the item was a PR review or a branch review. With only branch reviews remaining, the template is simplified to a single code path.

Affected files: crates/pika-git/templates/inbox.html

Evidence

@@ -134,10 +134,7 @@
           items.forEach(function (item) {
             var reviewId = item.review_id;
-            var isBranch = !!item.branch_id;
-            var titleText = isBranch
-              ? (escapeHtml(item.repo) + ' · branch ' + escapeHtml(item.branch_name || ('#' + reviewId)) + ': ' + escapeHtml(item.title))
-              : (escapeHtml(item.repo) + ' · legacy review #' + reviewId + ': ' + escapeHtml(item.title));
+            var titleText = escapeHtml(item.repo) + ' · branch ' + escapeHtml(item.branch_name) + ': ' + escapeHtml(item.title);

In inbox.html:134-140, the isBranch boolean check and the ternary expression that chose between branch and legacy-PR title formats are collapsed into a single format string:

var titleText = escapeHtml(item.repo) + ' · branch ' + escapeHtml(item.branch_name) + ': ' + escapeHtml(item.title);

The || ('#' + reviewId) fallback is also no longer needed because branch_name is now always a non-empty string.

Remove legacy state-directory and database migration logic from Nix module

Intent: The Nix pre-start script contained ~70 lines of shell code to handle migrating the state directory from /var/lib/pika-news to /var/lib/pika-git, choosing the richer database when duplicates existed, and creating a compatibility symlink. All of this is removed since the migration has long since completed on all deployments.

Affected files: infra/nix/modules/pika-git.nix

Evidence

@@ -6,90 +6,17 @@
   serviceGroup = "pika-git";
   gitUser = "git";
   serviceStateDir = "/var/lib/pika-git";
-  legacyServiceStateDir = "/var/lib/pika-news";
   canonicalGitDir = "${serviceStateDir}/pika.git";

@@ -6,90 +6,17 @@
-    count_table_rows() {
-      local db_path="$1"
-      local table_name="$2"
-    ...
-    db_richness_score() {
-      local db_path="$1"
-    ...

The prepareCanonicalRepo script in pika-git.nix is reduced from ~90 lines to ~10. Removed logic includes:

The legacyServiceStateDir variable (/var/lib/pika-news).
count_table_rows and db_richness_score helper functions that compared old vs. new databases.
Directory contents migration from legacy to current state dir.
Database file selection based on richness scoring.
Symlink creation from old to new path.

What remains is just mkdir -p, chown, and the canonical git repo existence check.

Rename SOPS secret keys from pika_news_ to pika_git_

Intent: The encrypted secrets file and the Nix expressions that reference them still used the legacy pika_news_ prefix for three secrets (GitHub token, Claude OAuth token, webhook secret). These are renamed to pika_git_ and the secrets file is re-encrypted.

Affected files: infra/nix/modules/pika-git.nix, infra/secrets/pika-git.yaml

Evidence

@@ -128,7 +55,7 @@
-  sops.secrets."pika_news_github_token" = {
+  sops.secrets."pika_git_github_token" = {

@@ -136,7 +63,7 @@
-  sops.secrets."pika_news_claude_oauth_token" = {
+  sops.secrets."pika_git_claude_oauth_token" = {

@@ -144,7 +71,7 @@
-  sops.secrets."pika_news_webhook_secret" = {
+  sops.secrets."pika_git_webhook_secret" = {

@@ -1,39 +1,39 @@
-pika_news_github_token: ENC[AES256_GCM,...]
-pika_news_claude_oauth_token: ENC[AES256_GCM,...]
-pika_news_webhook_secret: ENC[AES256_GCM,...]
+pika_git_github_token: ENC[AES256_GCM,...]
+pika_git_claude_oauth_token: ENC[AES256_GCM,...]
+pika_git_webhook_secret: ENC[AES256_GCM,...]

Three SOPS secret declarations in pika-git.nix are renamed:

Old key	New key
`pika_news_github_token`	`pika_git_github_token`
`pika_news_claude_oauth_token`	`pika_git_claude_oauth_token`
`pika_news_webhook_secret`	`pika_git_webhook_secret`

The environment file template (content = ''…'') is updated to reference the new placeholder paths. The SOPS-encrypted YAML file (infra/secrets/pika-git.yaml) is re-encrypted with the renamed top-level keys — the ciphertext changes but the plaintext values remain the same.

Update tests to verify legacy table cleanup and ledger rename

Intent: Ensure that after running all migrations, the legacy tables are gone, the new migrations ledger exists, and the old one does not. Also update existing tests that simulated a database with the old ledger name to use the LEGACY_MIGRATIONS_TABLE constant.

Affected files: crates/pika-git/src/storage.rs

Evidence

@@ -744,6 +783,17 @@
+                assert!(table_exists(conn, MIGRATIONS_TABLE)?);
+                assert!(!table_exists(conn, LEGACY_MIGRATIONS_TABLE)?);
+                assert!(!table_exists(conn, "pull_requests")?);
+                assert!(!table_exists(conn, "generated_artifacts")?);
+                assert!(!table_exists(conn, "artifact_versions")?);
+                assert!(!table_exists(conn, "artifact_user_states")?);
+                assert!(!table_exists(conn, "chat_sessions")?);
+                assert!(!table_exists(conn, "chat_messages")?);
+                assert!(!table_exists(conn, "inbox")?);
+                assert!(!table_exists(conn, "inbox_dismissals")?);
+                assert!(!table_exists(conn, "poll_markers")?);

@@ -978,8 +1032,8 @@
-        assert_eq!(items[0].branch_name.as_deref(), Some("feature/two"));
-        assert_eq!(items[1].branch_name.as_deref(), Some("feature/one"));
+        assert_eq!(items[0].branch_name, "feature/two");
+        assert_eq!(items[1].branch_name, "feature/one");

The test suite in storage.rs receives several updates:

Fresh-database test (storage.rs:783-793) — after running all migrations on a blank DB, asserts that _pika_git_migrations exists, _pika_news_migrations does not, and all nine legacy PR tables are absent.
Legacy-ledger upgrade tests — tests that previously hard-coded _pika_news_migrations now use the LEGACY_MIGRATIONS_TABLE constant (imported at storage.rs:685). After Store initialization, they assert the ledger was renamed to MIGRATIONS_TABLE and the legacy table no longer exists.
Inbox item assertions (storage.rs:1032-1033) — branch_name comparisons drop .as_deref() and Some(…) wrappers since the field is now a plain String.

pika-git-delete-legacy-pr-1

CI: success

Summary

Tutorial Steps

Add SQL migration to drop legacy PR-related tables

Rename the migrations ledger table from _pika_news_migrations to _pika_git_migrations

Simplify InboxItem struct — remove Option wrappers and PR fields

Remove the /news route alias from the web server

Remove localStorage legacy-key migration helpers from the frontend

Simplify inbox template — remove PR vs. branch rendering fork

Remove legacy state-directory and database migration logic from Nix module

Rename SOPS secret keys from pika_news_ to pika_git_

Update tests to verify legacy table cleanup and ledger rename

Diff