Fix Phase 1 migration: task table rebuild trips FK enforcement on populated DBs

migrate_add_reviewing_planner_output_status() rebuilds task (create-copy- drop-rename, SQLite's only way to change a CHECK constraint), but task is a parent table referenced by work_graph_run/artifact/execution_event/ bug_record. DROP TABLE task under PRAGMA foreign_keys=ON (schema.connect()'s default) trips FK enforcement the instant any child row exists -- unit tests never seeded a referencing child row, so this never surfaced until a real restart against the live production database (months of real history). Found and fixed live: no data loss (INSERT INTO task_new already succeeded before the DROP failed; verified the two tables held byte-identical rows, cleaned up the stray task_new, backed up the db, re-applied the fixed migration, verified via PRAGMA foreign_key_check, then restarted the real process against production and confirmed a real end-to-end task run through the new Planner -> planner_reviewer -> Coder -> Validator flow). Fix follows the migrate_relax_org_node_constraints() precedent already in this file: PRAGMA foreign_keys=OFF around the rebuild, PRAGMA foreign_key_check after, ON in a finally block. New regression test seeds one referencing row per child table so this class of bug can't pass silently again. Also adds real (non-mocked) coverage for the planner_reviewer role in test_e2e_pipeline_e2e.py: one real call confirming it flags real narrative- text repro fixtures, one confirming it passes real structured content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Fix Phase 1 migration: task table rebuild trips FK enforcement on populated DBs
migrate_add_reviewing_planner_output_status() rebuilds task (create-copy- drop-rename, SQLite's only way to change a CHECK constraint), but task is a parent table referenced by work_graph_run/artifact/execution_event/ bug_record. DROP TABLE task under PRAGMA foreign_keys=ON (schema.connect()'s default) trips FK enforcement the instant any child row exists -- unit tests never seeded a referencing child row, so this never surfaced until a real restart against the live production database (months of real history). Found and fixed live: no data loss (INSERT INTO task_new already succeeded before the DROP failed; verified the two tables held byte-identical rows, cleaned up the stray task_new, backed up the db, re-applied the fixed migration, verified via PRAGMA foreign_key_check, then restarted the real process against production and confirmed a real end-to-end task run through the new Planner -> planner_reviewer -> Coder -> Validator flow). Fix follows the migrate_relax_org_node_constraints() precedent already in this file: PRAGMA foreign_keys=OFF around the rebuild, PRAGMA foreign_key_check after, ON in a finally block. New regression test seeds one referencing row per child table so this class of bug can't pass silently again. Also adds real (non-mocked) coverage for the planner_reviewer role in test_e2e_pipeline_e2e.py: one real call confirming it flags real narrative- text repro fixtures, one confirming it passes real structured content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bboonnee · 宜博
1 parent 86fc0bbc
Showing 3 changed files with 161 additions and 32 deletions
src/schema.py
src/test_e2e_pipeline_e2e.py
src/test_schema.py
@@ -702,41 +702,69 @@ def migrate_add_reviewing_planner_output_status(conn):
     ALREADY-POPULATED database -- same create-copy-drop-rename dance as
     migrate_expand_artifact_types/migrate_add_loop_back_event_type. Idempotent
     and safe to call unconditionally, including against a freshly
-    create_schema()'d database (which already has the expanded constraint)."""
+    create_schema()'d database (which already has the expanded constraint).
+
+    **2026-08-12 real-restart incident fix**: `task` is a PARENT table --
+    work_graph_run/artifact/execution_event/bug_record all REFERENCES
+    task(task_id) -- unlike execution_event (a leaf table nothing points
+    at), which is why migrate_add_planner_review_verdict_event_type doesn't
+    need this. Rebuilding a referenced parent table with
+    `PRAGMA foreign_keys=ON` (schema.connect()'s default -- see its own
+    docstring) trips FK enforcement on the DROP TABLE step the instant any
+    child row exists, even though the surviving task_id column's type/PK
+    are unchanged. Caught for real restarting the live production
+    backend_server against ~/.graphlemon/graphlemon.db (months of real
+    work_graph_run/artifact/execution_event history) -- every unit test for
+    this migration had passed because none of them seeded a single
+    FK-referencing child row, so DROP TABLE never actually exercised FK
+    enforcement in-test. Now follows the exact
+    migrate_relax_org_node_constraints() precedent (that function's own
+    docstring explains the same PRAGMA foreign_keys=OFF/rebuild/
+    foreign_key_check/ON dance in more depth) instead of the leaf-table
+    executescript() shortcut this originally (wrongly) copied."""
     row = conn.execute("SELECT sql FROM sqlite_master WHERE type='table' AND name='task'").fetchone()
     if row is None or "'reviewing_planner_output'" in row[0]:
         return
-    conn.executescript(
-        """
-        CREATE TABLE task_new (
-            task_id TEXT PRIMARY KEY,
-            project_id TEXT NOT NULL REFERENCES project(project_id),
-            goal_text TEXT NOT NULL,
-            task_type TEXT NOT NULL CHECK (task_type IN ('goal', 'bug_fix')),
-            origin_ref TEXT,
-            created_via TEXT NOT NULL DEFAULT 'api' CHECK (created_via IN ('api', 'human_console')),
-            status TEXT NOT NULL CHECK (status IN (
-                'queued', 'planning_brd_prd_sdd', 'reviewing_planner_output', 'planning_failed',
-                'coding', 'coding_failed', 'validating',
-                'routed_fast_lane', 'routed_full_replan',
-                'blocked_pending_human', 'done'
-            )),
-            route TEXT CHECK (route IN ('fast_lane', 'full_replan')),
-            loop_round INTEGER NOT NULL DEFAULT 0,
-            blocked_reason TEXT,
-            assigned_org_node TEXT NOT NULL CHECK (assigned_org_node != ''),
-            created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
-            updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP
-        );
-        INSERT INTO task_new SELECT * FROM task;
-        DROP TABLE task;
-        ALTER TABLE task_new RENAME TO task;
-        CREATE INDEX idx_task_status ON task(status);
-        CREATE INDEX idx_task_assigned_org_node ON task(assigned_org_node);
-        CREATE INDEX idx_task_project_id ON task(project_id);
-        """
-    )
-    conn.commit()
+    conn.execute("PRAGMA foreign_keys = OFF")
+    try:
+        conn.executescript(
+            """
+            CREATE TABLE task_new (
+                task_id TEXT PRIMARY KEY,
+                project_id TEXT NOT NULL REFERENCES project(project_id),
+                goal_text TEXT NOT NULL,
+                task_type TEXT NOT NULL CHECK (task_type IN ('goal', 'bug_fix')),
+                origin_ref TEXT,
+                created_via TEXT NOT NULL DEFAULT 'api' CHECK (created_via IN ('api', 'human_console')),
+                status TEXT NOT NULL CHECK (status IN (
+                    'queued', 'planning_brd_prd_sdd', 'reviewing_planner_output', 'planning_failed',
+                    'coding', 'coding_failed', 'validating',
+                    'routed_fast_lane', 'routed_full_replan',
+                    'blocked_pending_human', 'done'
+                )),
+                route TEXT CHECK (route IN ('fast_lane', 'full_replan')),
+                loop_round INTEGER NOT NULL DEFAULT 0,
+                blocked_reason TEXT,
+                assigned_org_node TEXT NOT NULL CHECK (assigned_org_node != ''),
+                created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
+                updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP
+            );
+            INSERT INTO task_new SELECT * FROM task;
+            DROP TABLE task;
+            ALTER TABLE task_new RENAME TO task;
+            CREATE INDEX idx_task_status ON task(status);
+            CREATE INDEX idx_task_assigned_org_node ON task(assigned_org_node);
+            CREATE INDEX idx_task_project_id ON task(project_id);
+            """
+        )
+        conn.commit()
+        violations = conn.execute("PRAGMA foreign_key_check").fetchall()
+        if violations:
+            raise RuntimeError(
+                "migrate_add_reviewing_planner_output_status left dangling foreign keys: {}".format(violations)
+            )
+    finally:
+        conn.execute("PRAGMA foreign_keys = ON")
 def migrate_add_planner_review_verdict_event_type(conn):
@@ -93,5 +93,63 @@ class RealPrdConsistencyCheckTest(unittest.TestCase):
         self.assertTrue(ok, "false positive on a consistent PRD: {}".format(issues))
+@skip_unless_env("RUN_REAL_AGENT_E2E")
+class RealPlannerReviewTest(unittest.TestCase):
+    """Org Graph unification Phase 1, real coverage (2026-08-08): the new
+    planner_reviewer role is a genuinely new LLM-judgment capability (not a
+    wiring change) -- validated for real once each way (real narrative-text
+    repro / real structured content), same discipline as
+    RealPrdConsistencyCheckTest above. Also the cheapest real check that the
+    new role actually deploys correctly (.claude/agents/planner_reviewer.md
+    is discoverable and loadable by a real `claude --agent planner_reviewer`
+    call) -- if role.json/SOUL.md/AGENTS.md have a real wiring mistake, this
+    is where it surfaces, not deep inside a full run_end_to_end() call."""
+
+    def test_detects_real_narrative_text_instead_of_structured_content(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            subprocess.run(["git", "init"], cwd=tmp, check=True, capture_output=True)
+            agent_profile.deploy_agent_definitions(tmp)
+            conn = schema.connect(":memory:")
+            schema.create_schema(conn)
+            project_scaffold.create_project(conn, tmp, init_git_if_missing=False)
+            project_id = conn.execute("SELECT project_id FROM project").fetchone()[0]
+            task_id = task_create.create_task(conn, project_id, "实现贪吃蛇游戏")["task_id"]
+
+            # real repro text from docs/todo/validator-test-script-narrative-
+            # output-issue.md, fed in as the SDD content.
+            narrative_sdd = (
+                "Launched the 'graphlemon-node-f9cefb87af4c492aac738b9736d41e4e' workflow "
+                "(Work Node validator) in the background, run ID wf_93345ca8-8c6. It's running "
+                "asynchronously — I'll report results once it completes; no further action is "
+                "needed right now."
+            )
+            result = e2e_pipeline.review_planner_output(
+                conn, task_id, "实现贪吃蛇游戏", tmp, sdd_content=narrative_sdd,
+            )
+        self.assertIn(result["verdict"], ("FAIL", "INCONCLUSIVE"))
+
+    def test_passes_real_structured_content(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            subprocess.run(["git", "init"], cwd=tmp, check=True, capture_output=True)
+            agent_profile.deploy_agent_definitions(tmp)
+            conn = schema.connect(":memory:")
+            schema.create_schema(conn)
+            project_scaffold.create_project(conn, tmp, init_git_if_missing=False)
+            project_id = conn.execute("SELECT project_id FROM project").fetchone()[0]
+            task_id = task_create.create_task(conn, project_id, "实现一个返回两数之和的函数")["task_id"]
+
+            real_sdd = (
+                "# SDD: add(a, b)\n\n"
+                "## Module\n\n`add(a: int, b: int) -> int` in `math_utils.py`.\n\n"
+                "## Behavior\n\nReturns the sum of the two arguments. No validation needed -- "
+                "both arguments are guaranteed to be integers by the caller.\n\n"
+                "## Tests\n\n`test_add_positive`, `test_add_negative`, `test_add_zero`.\n"
+            )
+            result = e2e_pipeline.review_planner_output(
+                conn, task_id, "实现一个返回两数之和的函数", tmp, sdd_content=real_sdd,
+            )
+        self.assertEqual(result["verdict"], "PASS", result["raw_response"])
+
+
 if __name__ == "__main__":
     unittest.main()
@@ -871,6 +871,31 @@ class MigrateAddReviewingPlannerOutputStatusTest(unittest.TestCase):
             "INSERT INTO task (task_id, project_id, goal_text, task_type, status, assigned_org_node) "
             "VALUES ('task-1', 'proj-1', 'implement snake game', 'goal', 'queued', 'planner')"
         )
+        # 2026-08-12 real-restart incident regression: task is a PARENT table --
+        # every real production database has months of child rows across these
+        # four tables REFERENCES task(task_id). Seeding at least one of each here
+        # is the actual fix for why the original migration's bug (DROP TABLE task
+        # under PRAGMA foreign_keys=ON trips FK enforcement the instant a
+        # referencing child row exists) went undetected by this test class --
+        # every test below previously ran against a task row with ZERO
+        # referencing children, so FK enforcement was never truly exercised.
+        self.conn.execute(
+            "INSERT INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) "
+            "VALUES ('run-1', 'task-1', 'planner', '{}', 'done')"
+        )
+        self.conn.execute(
+            "INSERT INTO artifact (artifact_id, artifact_type, task_id, produced_by_run_id, "
+            "file_path, git_commit_sha, validation_status) "
+            "VALUES ('art-1', 'sdd', 'task-1', 'run-1', 'f.md', 'sha', 'passed')"
+        )
+        self.conn.execute(
+            "INSERT INTO execution_event (event_id, project_id, task_id, event_type, payload) "
+            "VALUES ('evt-1', 'proj-1', 'task-1', 'task_status_changed', '{}')"
+        )
+        self.conn.execute(
+            "INSERT INTO bug_record (bug_id, task_id, source, description, status) "
+            "VALUES ('bug-1', 'task-1', 'validator', 'real bug', 'open')"
+        )
         self.conn.commit()
     def tearDown(self):
@@ -882,6 +907,24 @@ class MigrateAddReviewingPlannerOutputStatusTest(unittest.TestCase):
                 "UPDATE task SET status = 'reviewing_planner_output' WHERE task_id = 'task-1'"
             )
+    def test_migration_succeeds_with_real_foreign_key_referencing_child_rows(self):
+        # the actual regression: must not raise sqlite3.IntegrityError
+        # (FOREIGN KEY constraint failed) -- this is exactly what happened
+        # restarting the live production backend_server for real.
+        schema.migrate_add_reviewing_planner_output_status(self.conn)
+        for table, id_col, id_val in (
+            ("work_graph_run", "run_id", "run-1"),
+            ("artifact", "artifact_id", "art-1"),
+            ("execution_event", "event_id", "evt-1"),
+            ("bug_record", "bug_id", "bug-1"),
+        ):
+            row = self.conn.execute(
+                "SELECT task_id FROM {} WHERE {} = ?".format(table, id_col), (id_val,)
+            ).fetchone()
+            self.assertEqual(row[0], "task-1", "{} lost its task_id linkage after migration".format(table))
+        violations = self.conn.execute("PRAGMA foreign_key_check").fetchall()
+        self.assertEqual(violations, [])
+
     def test_migration_preserves_existing_rows_and_accepts_new_status(self):
         schema.migrate_add_reviewing_planner_output_status(self.conn)