fix(data): publish migration and cursor corrections (successor to #185)

[PeterGuy326]

Refs #178.

After #197 closed

#197 was superseded by #219 (0025 HNSW on main). This PR no longer stacks on codex/fix-pr-180. It is rebased onto current main and keeps only the #178 data-plane hygiene slice:

• migration 0026: text-chunk uniqueness, partial indexes on nullable memory FKs, cascade memory_relations with workspace deletion
• batched generation listings
• opaque cursor pagination for relation listings

Head: ac4a3c9 (3 cherry-picks: 5cbd05b → rename 0026 → cursor/populated verification). Lexical/HNSW files that were already on main were dropped.

Not claimed: live production latency, or that #197 merged.

[PeterGuy326]

Independent automated preflight: HOLD on deployment order at 746501415f7e7f56f09bcd21f349b5adb9a9cf24.

The source tree has migrations 1–23 and 26, without 24/25. The runner uses Goose without WithAllowMissing, so deploying this candidate before the lower numbered migrations makes their later introduction invalid. This is source-confirmed, not a newly executed PostgreSQL upgrade test. An enforced deployment sequence or coordinated, verified-unreleased numbering is required before this PR can be treated as independently deployable.

The actual cursor, populated uniqueness, and bounded-query fixes passed independent source/compile checks; the implementation worker's real PostgreSQL evidence remains separately attributed. Latest 20/20 successful checks do not prove the cross-PR upgrade sequence. Follow-up is assigned, with no migration-policy waiver or deployed-history rewrite authorized.

[sun-970]

Code Review

Overall code quality is good and test coverage is solid. Here are a few specific issues and suggestions:

---

1. [Medium] Migration 0026's DELETE deduplication may be slow on large datasets

DELETE FROM embeddings_text
 WHERE id NOT IN (
    SELECT DISTINCT ON (file_id, chunk_index) id
      FROM embeddings_text
     ORDER BY file_id, chunk_index, id
 );

NOT IN (SELECT ...) requires materializing the IDs of all rows to keep. If the embeddings_text table is large, this holds a table lock for an extended period. The PR description also acknowledges that "write path DELETEs all chunks before re-inserting, so duplicates should never exist in practice."

Suggestion: Add a pre-check to skip the DELETE if there are no duplicates:

DO $$
BEGIN
  IF EXISTS (
    SELECT 1 FROM embeddings_text
    GROUP BY file_id, chunk_index
    HAVING count(*) > 1
    LIMIT 1
  ) THEN
    DELETE FROM embeddings_text
     WHERE id NOT IN (
      SELECT DISTINCT ON (file_id, chunk_index) id
        FROM embeddings_text
       ORDER BY file_id, chunk_index, id
     );
  END IF;
END $$;

This way, on the vast majority of production tables that have no duplicates, the migration only needs a lightweight GROUP BY check to pass.

---

2. [Low] Two valuable comments were removed

These two comments were removed from relation.go:

// The anchor memory must exist, be visible under the caller's allowed
// paths, and not be forgotten — mirroring Get — so listing relations never
// leaks the existence of a memory the caller cannot read.

// BFS from target following supersedes/corrects edges in their forward
// direction: if we ever reach sourceID, there is a cycle.

The first explains the existence-leakage guard for the security check — this is critical context for understanding why the anchor check is necessary. The second explains the BFS strategy of wouldCycle. Both are "WHY is non-obvious" comments and should be preserved.

---

3. [Low] Three unrelated fixes bundled in one migration

The PR description says "each is a single DDL statement and none warrants its own schema version." However, Item 3 (the FK cascade change) is semantically the heaviest — it drops and recreates three foreign key constraints. If a deployed environment has manually adjusted these FKs (e.g., temporarily changed to DEFERRABLE), this migration would overwrite them.

Consider documenting this assumption in the PR description or as a migration comment.

---

4. [Nit] relationFilterHash JSON + SHA-256 approach

payload := relationFilterFingerprint{...}
encoded, _ := json.Marshal(payload)
sum := sha256.Sum256(encoded)

JSON serialization + SHA-256 for generating the filter fingerprint has the benefit of staying consistent with the cursor format in list.go, but JSON marshaling introduces some fragility (field ordering, zero-value handling, etc.). Since this is only for preventing cursor reuse across queries and not for security purposes, a simpler approach (e.g., direct string concatenation before hashing) could work. That said, the current approach is acceptable for consistency.

---

What's done well

• N+1 fix (service.go): Clean and straightforward — collects buildIDs first, then batch-queries in one go. The test verifies exactly 2 SQL statements.
• Migration tests (migrations_test.go): Drops the constraint inside a rollback-only transaction → inserts duplicates → replays the migration, verifying both deduplication and the unique constraint. The _test suffix check on the database name is also a good safety practice.
• Cursor pagination: The filter hash binding prevents cursor reuse across filters. The limit+1 pattern for determining whether there's a next page is the standard keyset pagination approach. Using id > as a tiebreaker for tied timestamps is correct (paired with ORDER BY created_at DESC, id ASC).
• Down migration: Correctly reverses all changes in proper order.

[waterbro-8]

Code review（draft，给作者的清单，非批准）

一、keyset 分页：形状是对的，而且做对了一件容易漏的事

我特意核了排序与游标谓词是否自洽，因为这是这类改动最常错的地方：

• 谓词 (r.created_at < $t OR (r.created_at = $t AND r.id > $id))，而 SQL 的排序是 ORDER BY r.created_at DESC, r.id（id 升序）。两者一致，不会出现"同一时间戳的行被跳过或重复"。这条我对照了 main 上的原实现与这条分支的实现，排序没有被改动，新增的谓词是配得上它的。
• 取 q.Limit+1 行来探测是否还有下一页，len(out) <= q.Limit 时不出 next_cursor。✓
• 游标取自裁剪后那一页的最后一行（result.Relations[len-1]），不是 out[q.Limit]——这点很容易写错，这里是对的。
• 游标绑定了过滤条件的哈希（workspace + memory + direction + relationType 四项参与 sha256）。这是我认为最值得记一笔的地方：它让一个游标无法被拿到另一组过滤条件下重放。很多 keyset 实现会漏掉这个约束，导致跨查询的游标串味。
• 非法游标走 ErrInvalidCursor → handler 映射成 400 invalid_cursor，不是 500。✓

二、0026 迁移：三条都是能站住的，而且写清楚了取舍

• Item 1（唯一性）：先按 DISTINCT ON (file_id, chunk_index) ... ORDER BY file_id, chunk_index, id 去重，再加 UNIQUE (file_id, chunk_index)。注释明说"保留 UUID 最小的那一行是为了确定性选择，UUID 顺序不等于插入顺序"——这句话很关键，它承认了去重规则是人为约定而非"保留原始那行"。我不认为这是缺陷（写入路径本来就是先删后插，理论上不会有重复），但这个诚实度值得保持。
• Item 2（部分索引）：idx_memories_source_file_id 与 idx_memories_created_by_user_id 都是 WHERE ... IS NOT NULL 的部分索引，正是 ON DELETE SET NULL 反查需要的形状，而且部分索引不会为空值浪费空间。顺带一提：source_file_id 这个索引正好也是我在 #210 上提出的那条守卫缺陷（只查 memories.path、不查 source_file_id）如果要修，所需要的底层支撑——两条 PR 在这点上是有协同的。
• Item 3（级联对齐）：memory_relations 的三个 FK 改为与 memories.workspace_id 一致的 ON DELETE CASCADE。理由写得很实在：memories 会被 workspace 删除连带清掉，而 memory_relations 不级联的话，删 workspace 会因为还残留引用它的边而失败。这是真的会咬人的不一致。
• Down 把三处 FK 还原成无级联、删掉索引、删掉约束，顺序与 Up 相反。✓

三、非阻塞，但建议补一句的两点

• 这次迁移在生产库上不是无感的，而且它碰的是最大的那张表。 Item 1 的 DELETE ... WHERE id NOT IN (...) 加 ADD CONSTRAINT UNIQUE，都要全表扫描 embeddings_text 并加锁（约束建立期间对写入是排他的）。这不是缺陷，但 docs/MIGRATION_SEQUENCE.md 目前只讲了顺序与严格性，没讲迁移窗口。考虑到 0024（重写 files + 建索引）、0025（在三个 embeddings_* 上建 HNSW）、0026（又是 embeddings_text）是连着跑的，建议在序列文档里补一句"这三次迁移会依次对最大的几张表加锁，请预留窗口"。
• Item 1 的 Down 是不保数据的（被去重删掉的行不会回来），迁移注释里没提。这是 down 迁移的常态，我不反对；只是既然注释对其他取舍都写得很细，这里补一句"down 不恢复被去重的行"会更完整。

四、接口变更的处理是完整的

ListRelations 的 Go 签名从 ([]Relation, error) 改成 (*ListRelationsResult, error)，涉及三处：api.go 的 MemoryService 接口、handlers_memory.go 的实际调用、以及 handlers_memory_test.go 里的 stub——三处都改了，没有留下编译不过的悬空实现。HTTP 层面是向后兼容的：relations 字段仍在，只是新增了可选的 next_cursor 与可选的 cursor 查询参数。

五、我没有声称验证的事

本机没有 PostgreSQL，我没有跑过 migrations_test.go、也没有跑过分页的集成测试。以上结论来自逐行读迁移、SQL 与分页逻辑，不是运行时观测。

另外需要点明：这条 PR 的 CI 绿不等于它可以先合。docs/MIGRATION_SEQUENCE.md 已经写明它是链上第 3 环（#195 HOLD behind #197），我同意这个约束——0026 声明的前驱是 schema 25，跳序部署会让 Goose 正确地在启动时失败。

（本次为独立评审：我不是本 PR 作者，也不是其 head 的推送者。draft 状态下我不投批准票。）

[waterbro-8]

Draft + dirty/conflicting. In the recorded order this waits on #197. Not merging drafts. Please rebase after #197 or close if superseded.

[xiaocui-big]

平台组 Review（崔泽生，assigned by @冯浩然）

一、总体评价

结构清晰、文档充分的 PR，解决 #178 五项数据面卫生中的三项（migration 0026），同时修复关系分页游标方向 bug、index generation listing N+1 查询。CI 22/22 全绿。PR body 明确标注"非独立可部署"和 HOLD behind #197。

二、Migration 0026 正确性

Item 1：embeddings_text 唯一性约束

• DISTINCT ON (file_id, chunk_index) ... ORDER BY file_id, chunk_index, id 保留 UUID 最小行，注释诚实说明"UUID 顺序不等于插入顺序，是确定性选择而非保留原始行" ✅
• DELETE + ADD CONSTRAINT 在同一事务内，约束失败则全回滚 ✅
• 关注：Down 不恢复被去重的行，建议补一句注释说明

Item 2：memories 部分索引

• WHERE source_file_id IS NOT NULL / WHERE created_by_user_id IS NOT NULL 正是 ON DELETE SET NULL 反查所需形状 ✅
• 无问题，三项中最干净

Item 3：memory_relations FK 级联对齐

• workspace 删除 ON DELETE CASCADE 到 memories，如果 relations 不级联会因残留边失败——修复正确 ✅
• Down 把三个 FK 还原为 NO ACTION，顺序正确 ✅

Up/Down 对称性：

Up	Down	对称
DELETE duplicates + ADD UNIQUE	DROP CONSTRAINT	部分（被删行不恢复）
CREATE INDEX x2 (partial)	DROP INDEX x2	✓
DROP + ADD FK (CASCADE) x3	DROP + ADD FK (NO ACTION) x3	✓

三、memory/relation.go 游标分页——核心 bug 修复正确

排序 ORDER BY r.created_at DESC, r.id（id 升序），游标谓词 (r.created_at < $t OR (r.created_at = $t AND r.id > $id))——DESC 排序需要"更小时间戳"或"同时间戳更大 UUID"，谓词一致 ✅

relationFilterFingerprint 将四个过滤维度 sha256 后嵌入游标，防止跨查询条件重放——少见的安全意识 ✅

集成测试验证了游标跨 filter 失效、同时间戳分页不重复不遗漏 ✅

被删除的注释：同意 sun-970 的判断——anchor memory 安全不变量说明和 wouldCycle BFS 策略说明是"WHY is non-obvious"注释，建议恢复。

四、indexgeneration/service.go N+1 修复——干净

listGenerationsForBuilds 用 build_id = ANY($2::uuid[]) 批量查询，集成测试通过 pgx Tracer 精确验证 List 只用 2 条 SQL（1 builds + 1 generations）✅

五、词法搜索路由（附加变更）

RouteLexical 使用与 Recall 相同的三层结构（精确短语 → FTS → trigram），不需要 worker。managed_embeddings.go 正确跳过 lexical 路由的 embedding provider 调用 ✅

strpos(lower(f.name), lower($N)) 无法利用索引，当前阶段可接受。

六、依赖链

main (schema 23) → #194 (0024) → #197 (0025) → #195 (0026)

Goose 严格模式，跳序部署会正确失败。三个 PR 连续对大表加锁，生产部署需预留维护窗口。

七、建议级问题

项	级别
恢复 relation.go 被删的安全注释	Medium
MIGRATION_SEQUENCE.md 补充迁移窗口提示	Medium
Item 1 去重加 IF EXISTS (duplicates) 前置检查	Low
Item 1 Down 注释补"不恢复被删行"	Low
Item 3 迁移注释加 FK 假设声明	Low

八、核心判断

代码质量高，变更正确，测试覆盖充分。依赖链管理严格，文档诚实。Draft 状态下不投批准票。等待 #197 先行解决、conflict 解决后，建议合并。

[waterbro-8]

崔泽生：质量过关，等 #197。同意部署顺序 #194(已合) → #197 → #195。

当前 DIRTY/CONFLICTING，rebase 需要作者 @PeterGuy326，且必须在 #197 的 0025 落地之后。

[waterbro-8]

#197 已关（被 #219 取代）。本 PR 已换基到 main，只保留 #178 的 0026 hygiene + relation cursor。

Head ac4a3c9。请审这一条独立切片，不要再当 HOLD behind #197。

[waterbro-8]

复审 exact head ac4a3c9512ec9066ce034b138c4f7b6bd0eb5182。migration 0026、N+1 batch、relation keyset 的实现形状均正确；排序 created_at DESC, id ASC 与 cursor 谓词 created_at < / id > 一致，filter hash 也绑定了 workspace/memory/direction/type。

当前仍缺 #178 的显式 Acceptance，暂不能批准：

• #178 要求 item 2 在 populated DB 上验证，并记录 ON DELETE SET NULL 路径加索引前后的 EXPLAIN。当前新增的 migrations_test.go 只验证 item 1 的去重/唯一约束；PR/issue 也没有任何 EXPLAIN 证据。请补可复现的 populated-DB 验证与 before/after plan（测试或验证 ledger 均可，但须能对应 source_file_id，且说明 created_by_user_id 是否同样验证）。
• #178 当前仍为 status:needs-triage。仓库 AGENTS.md 要求 material change 在 status:ready 后才能合入，需 requirement owner 先完成 ready 决策。

此前该 head 只有 PR Policy，并非完整 CI；我已在 exact branch 手动触发 CI、Validate Agent memory、Security Baseline，运行中。待上述 AC/治理项补齐且新 head checks 通过后再复审。

[waterbro-8]

补充 exact-head 验证：我手动触发的三套 workflow 已全部结束。

• CI: success（含 Go race/coverage、migration、build、Web/Worker/npm/Windows audit）
• Validate Agent memory: success（含 PostgreSQL integration）
• ByteFolk Security Baseline: CodeQL go/python/javascript-typescript 全 success；Dependency review 因 workflow_dispatch 条件按设计 skipped，本 PR 无依赖文件变更

Runs: https://github.com/bytefolk/mem/actions/runs/35425019338 · https://github.com/bytefolk/mem/actions/runs/35425019201 · https://github.com/bytefolk/mem/actions/runs/35425019538

这解决了 current head 原本缺失的运行验证，但不替代上一条 review 所列 #178 EXPLAIN / populated-index evidence 与 ready 决策。

[xiaocui-big]

平台组正式 Approve（崔泽生）

Re-review on head ac4a3c9。此前 9/18 的详细 review 结论不变：migration 0026 三项变更正确、游标分页谓词与排序一致、filter hash 绑定防跨条件重放、N+1 修复干净。

当时未投批准票是因为 PR 处于 Draft 且 HOLD behind #197。现在：

• #197 已被 #219 取代并合入 main
• 本 PR 已 rebase 到最新 main，只保留 #178 hygiene 切片
• CI 全绿（含 Go race/coverage、migration、PostgreSQL integration、Security Baseline）
• mergeable: MERGEABLE

代码层面的判断没有变化——实现正确，测试覆盖充分，依赖链管理严格。

关于 waterbro-8 提出的验收项

1. #178 status:ready 决策 — 同意这是治理项而非代码缺陷。维护者需完成 triage 决策。
2. Populated DB EXPLAIN 证据 — 合理要求，但不阻塞代码正确性判断。可在合并后由 follow-up 补齐，或由 requirement owner 在 triage 时一并记录。

代码已就绪，批准。

[PeterGuy326]

Populated-database verification for #178 item 2 is now recorded against exact head ac4a3c9512ec9066ce034b138c4f7b6bd0eb5182. This covers both source_file_id and created_by_user_id, using actual parent DELETEs and the resulting PostgreSQL foreign-key trigger queries.

Environment and result

PostgreSQL 17.10, pgvector 0.8.3, pg_trgm 1.6, disposable local database. Applied the unmodified Up SQL from migrations 0001–0025 at this head, populated the fixture, captured the baseline, then applied the unmodified 0026 Up and repeated the same checks. Each migration ran in its own transaction through psql; this was SQL validation, not a new Goose/Go acceptance run.

Fixture: 100,000 memories; each tested FK has 10,000 non-NULL values, including 100 references to the target parent, and 90,000 NULLs. Planner scan options remained enabled; no forced-index setting was used.

Actual FK lookup	Before 0026	After 0026	Rows returned	Before rows filtered	Scan buffers before → after
source_file_id	Seq Scan	idx_memories_source_file_id Index Scan	100	99,900	5,920 hits → 7 hits + 2 reads
created_by_user_id	Seq Scan	idx_memories_created_by_user_id Index Scan	100	99,900	5,925 hits → 17 hits

These are child scan-node buffers, not total DELETE/UPDATE buffers. All four DELETE cases preserved the 100,000 memories, nulled exactly 100 matching references, and restored those references on rollback.

Reproduction

Use an isolated test database with the extensions above. Extract each migration from this exact Git commit and take only the text before -- +goose Down; execute 0001–0025 in numeric order, wrapping each Up section in BEGIN/COMMIT. Then load this synthetic fixture:

INSERT INTO users(id,email,password_hash) VALUES
 ('10000000-0000-0000-0000-000000000001','owner@fixture.invalid','fixture'),
 ('10000000-0000-0000-0000-000000000002','target@fixture.invalid','fixture');
INSERT INTO workspaces(id,name,resource_owner_user_id) VALUES
 ('20000000-0000-0000-0000-000000000001','fixture','10000000-0000-0000-0000-000000000001');
INSERT INTO files(id,user_id,name,path,size,sha256,storage_key) VALUES
 ('30000000-0000-0000-0000-000000000001','10000000-0000-0000-0000-000000000001','target','/target',0,repeat('1',64),'fixture:target'),
 ('30000000-0000-0000-0000-000000000002','10000000-0000-0000-0000-000000000001','other','/other',0,repeat('2',64),'fixture:other');
INSERT INTO memories(id,workspace_id,created_by_user_id,source_file_id,
 kind,content,path,source_type,idempotency_key_sha256,request_sha256,content_sha256)
SELECT md5('mem195-'||i::text)::uuid,
 '20000000-0000-0000-0000-000000000001',
 CASE WHEN i<=100 THEN '10000000-0000-0000-0000-000000000002'::uuid
      WHEN i<=10000 THEN '10000000-0000-0000-0000-000000000001'::uuid END,
 CASE WHEN i<=100 THEN '30000000-0000-0000-0000-000000000001'::uuid
      WHEN i<=10000 THEN '30000000-0000-0000-0000-000000000002'::uuid END,
 'note','synthetic local FK fixture '||i::text,'/fixture','test',
 repeat(md5(i::text),2),repeat('0',64),repeat('0',64)
FROM generate_series(1,100000) AS i;
INSERT INTO embeddings_text(id,file_id,chunk_index,chunk_text) VALUES
 ('40000000-0000-0000-0000-000000000001','30000000-0000-0000-0000-000000000002',0,'original'),
 ('40000000-0000-0000-0000-000000000002','30000000-0000-0000-0000-000000000002',0,'legacy duplicate');
ANALYZE;

Enable nested-plan capture in the same privileged local test session:

LOAD 'auto_explain';
SET auto_explain.log_min_duration=0;
SET auto_explain.log_nested_statements=on;
SET auto_explain.log_analyze=on;
SET auto_explain.log_buffers=on;
SET auto_explain.log_timing=off;
SET auto_explain.log_format=json;
SET auto_explain.log_level=notice;

BEGIN;
EXPLAIN (ANALYZE, BUFFERS, TIMING OFF, FORMAT JSON)
DELETE FROM files WHERE id='30000000-0000-0000-0000-000000000001';
SELECT count(*), count(source_file_id) FROM memories; -- 100000, 9900
ROLLBACK;
BEGIN;
EXPLAIN (ANALYZE, BUFFERS, TIMING OFF, FORMAT JSON)
DELETE FROM users WHERE id='10000000-0000-0000-0000-000000000002';
SELECT count(*), count(created_by_user_id) FROM memories; -- 100000, 9900
ROLLBACK;

Apply 0026 Up on this populated fixture, run ANALYZE memories, and repeat those two transactions. Capture both psql stdout and stderr. In each NOTICE: duration ... plan: JSON, select Query Text beginning with the respective statement below, then inspect Plan.Plans[0]. This captures the actual referential-action UPDATE, not an equivalent manually issued query:

UPDATE ONLY "public"."memories" SET "source_file_id" = NULL
WHERE $1 OPERATOR(pg_catalog.=) "source_file_id";
UPDATE ONLY "public"."memories" SET "created_by_user_id" = NULL
WHERE $1 OPERATOR(pg_catalog.=) "created_by_user_id";

The four observed scan excerpts, with unrelated JSON fields omitted:

[
 {"case":"before/source_file_id","Node Type":"Seq Scan","Actual Rows":100,"Actual Loops":1,"Rows Removed by Filter":99900,"Shared Hit Blocks":5920,"Shared Read Blocks":0},
 {"case":"after/source_file_id","Node Type":"Index Scan","Index Name":"idx_memories_source_file_id","Actual Rows":100,"Actual Loops":1,"Rows Removed by Index Recheck":0,"Shared Hit Blocks":7,"Shared Read Blocks":2},
 {"case":"before/created_by_user_id","Node Type":"Seq Scan","Actual Rows":100,"Actual Loops":1,"Rows Removed by Filter":99900,"Shared Hit Blocks":5925,"Shared Read Blocks":0},
 {"case":"after/created_by_user_id","Node Type":"Index Scan","Index Name":"idx_memories_created_by_user_id","Actual Rows":100,"Actual Loops":1,"Rows Removed by Index Recheck":0,"Shared Hit Blocks":17,"Shared Read Blocks":0}
]

Additional migration checks and limits

0026 Up succeeded on populated data. The duplicate chunk fixture retained UUID 40000000-0000-0000-0000-000000000001; inserting another (file_id, chunk_index) duplicate was rejected with SQLSTATE 23505. The original 0026 Down also executed successfully inside a rollback-only transaction, removing both new indexes. Down does not restore previously deduplicated rows.

This establishes the two requested FK lookup plans on the stated fixture. It does not establish production latency, concurrency/locking behavior, or PostgreSQL 16 performance; repository CI uses PG16, separately from this local PG17 run. A complete user DELETE still triggered another sequential scan for memories.forgotten_by_user_id, outside these two indexes. Indexes do not eliminate UPDATE locks or other trigger costs.

No implementation change was made. #178 still needs the maintainer's readiness decision, and the existing change request needs the original reviewer's re-review. This evidence and another reviewer's approval do not substitute for either decision. The old #197 dependency is no longer the blocker.

[waterbro-8]

复审 exact head ac4a3c9512ec9066ce034b138c4f7b6bd0eb5182（无新 commit）。不是验收。未合。

技术阻断（#178 item 2 populated EXPLAIN）已解除： 评论里的 10 万行 fixture、真实父 DELETE 触发的两条 FK UPDATE、加索引前后 plan 对应 source_file_id 与 created_by_user_id（Seq Scan → 对应 Index Scan，100 行 / 99900 filtered）。这是 SQL 校验，不是 Goose/Go 新验收，也不是 PG16 CI 或生产时延；forgotten_by_user_id 仍是 Seq Scan，已如实写出。可以对应这项 AC。

仍不能 Approve 为可合入：

• #178 仍是 status:needs-triage。AGENTS.md 要求 material change 在 status:ready 后才能合。#178 ready 是维护者决策，评论证据和另一人的 Approve 都不能替代。

合入是你。我不合。

[waterbro-8]

Fresh review on exact head 71db9ab67acb22e25f26e958059b2ed8058e0624. The five #178 changes remain scoped and technically sound. Latest main is integrated without rewriting published history; main owns migration 0026, so data-plane hygiene is correctly sequenced as 0027. TestMigrationUpgradeSequence now verifies schema 26 producer-agent indexing before schema 27 deduplication/unique rejection. The issue is status:ready, populated PostgreSQL EXPLAIN evidence is recorded, the branch is mergeable, and all 21 hosted checks pass. Approving for squash merge; this is not a production-latency claim.

[waterbro-8]

Fresh formal review on exact head 71db9ab67acb22e25f26e958059b2ed8058e0624.

No merge-blocking finding:

• latest main owns migration 0026 and this branch correctly sequences hygiene as 0027; TestMigrationUpgradeSequence verifies 26 producer-agent indexing before 27 dedupe/unique rejection;
• 0027 Up deterministically deduplicates (file_id, chunk_index), adds the unique constraint and two partial FK indexes, and aligns all three relation FKs with cascade; Down reverses constraints/indexes in safe order (deduplicated rows are intentionally not reconstructible);
• populated migration/duplicate 23505 coverage and recorded EXPLAIN evidence satisfy #178 item 1/2 evidence;
• relation keyset uses created_at DESC, id ASC with the matching < timestamp / > id predicate, takes limit+1, emits cursor from the retained page tail, and binds workspace/memory/direction/type via the existing cursor codec;
• index-generation List is two queries regardless of build count and maps each generation back by build/workspace, with integration query-count coverage.

#178 is now status:ready; all 21 hosted checks on this exact head are green. APPROVED for the configured squash auto-merge. This is not a production-latency claim.

[waterbro-8]

@Bindy-lbb 最后一项 branch-policy gate：请对 exact head 71db9ab67acb22e25f26e958059b2ed8058e0624 做正式 review。#178 已 ready，migration 已从冲突的 0026 顺延为 0027，21/21 checks 全绿；现有独立技术复审不被规则识别为指定 CODEOWNER。已开 squash auto-merge。

[sun-970]

Reviewed the diff from outside — I do not hold write access on this repository, so this is a comment rather than an approval. A maintainer with write access still needs to give the formal verdict.

Pagination is correct. At head, the query orders by r.created_at DESC, r.id (line 337), which exactly matches the new predicate (r.created_at < $t OR (r.created_at = $t AND r.id > $i)); next_cursor is built from the last row of the trimmed page after fetching limit+1. No skip/duplicate at equal timestamps. Binding the cursor to a sha256 fingerprint of (workspace, memory, direction, relationType) is a good touch — a cursor cannot be replayed against a different filter, and mismatch is rejected as ErrInvalidCursor → 400.

Also good: the response stays backwards compatible ({relations} → {relations, next_cursor?}), a nil slice still serializes as [], and the N+1 in indexgeneration.Service.List is replaced by one = ANY($2::uuid[]) batch query plus in-memory grouping — same result, one round trip.

Migration 0027:

• The dedup before ADD CONSTRAINT keeps the smallest id per (file_id, chunk_index) with a deterministic ORDER BY, and the comment is honest that UUID order is not insert order. Correct.
• Aligning the memory_relations FKs to ON DELETE CASCADE matches the stated root cause (workspace delete cascades memories, then fails on the relation edges), and the Down body re-adds the FKs without an action — the right inverse.
• One operational note: ALTER TABLE … ADD CONSTRAINT UNIQUE holds ACCESS EXCLUSIVE while the index builds (CONCURRENTLY is not available for a constraint added this way). On a large embeddings_text this briefly blocks writes — worth a line in the migration comment if deploy windows matter.

[Bindy-lbb]

Reviewed exact head 71db9ab67acb22e25f26e958059b2ed8058e0624 against current main 73c316a (14/14 changed files inspected; OCR selected 7 production files, and I also inspected the 7 excluded test/documentation files).

One merge-blocking finding: migration version 0027 now collides with main. Main commit 8c4080a (#235) added server/internal/db/migrations/0027_vault_sync.sql, while this PR adds 0027_data_plane_hygiene.sql. The files do not create a Git filename conflict, so MERGEABLE and the old 21 green checks do not establish migration compatibility.

I reproduced this without a database by assembling the unchanged SQL migrations from current main plus this PR's hygiene migration and calling the repository-pinned Goose v3.22.1 CollectMigrations. Goose rejects the set with goose: duplicate version 27 detected, naming both files. Startup uses Goose over all embedded migrations.

Please integrate current main and move the unmerged hygiene migration to the next available version (currently 0028), updating EXPECTED_MIGRATION_HEAD, migration-sequence assertions, test fixture filenames, and migration documentation together. Rerun migration/upgrade and PostgreSQL integration checks on the updated head. Preserve already released migration identities; privately applied draft versions still need an operator-owned recovery plan.

Local validation on this PR head: go test ./internal/api ./internal/memory ./internal/indexgeneration ./internal/db passed. I did not rerun live PostgreSQL integration or production performance checks. The batching and cursor changes have no other blocking finding from this review. Withholding approval until the migration collision is resolved.

[sun-970]

Blocking finding reproduced; fix is in #237

@Bindy-lbb I reproduced the migration collision with the same method and confirmed your root cause. Main 8c4080a (#235) added 0027_vault_sync.sql; this branch carries 0027_data_plane_hygiene.sql.

Assembling the unmodified SQL from current main plus this PR's hygiene migration and calling the repository-pinned Goose v3.22.1 CollectMigrations:

panic: goose: duplicate version 27 detected:
  .../0027_vault_sync.sql
  .../0027_data_plane_hygiene.sql

Your two points both hold: it blocks migration, and the previous 21 green checks did not cover this combination. MERGEABLE and the filename-level diff never established migration compatibility.

Fix

Because I only hold read access on bytefolk/mem I cannot push to codex/fix-pr-185. The fix is carried in #237, based on this PR's head branch, so it can be merged or cherry-picked in without rewriting published history:

• rename 0027_data_plane_hygiene.sql -> 0028_data_plane_hygiene.sql
• migrations_test.go fixture path follows the rename
• migration_sequence_test.go version gates >= 27 -> >= 28 (sequence still asserts 26 producer-agent indexing -> 27 vault_sync -> 28 dedupe/unique rejection)
• docs/MIGRATION_SEQUENCE.md: predecessor schema 27, head 24 through 28, and a note on why the draft moved twice (0026 -> 0027 -> 0028)

Released migration identities are preserved - 0027 now belongs to main's vault_sync.

Verification on the fixed head (Go 1.27.0, Windows)

• CollectMigrations over the assembled tree: 28 migrations, no duplicate version, head 0028_data_plane_hygiene.sql
• go build ./... clean
• go test ./internal/db/... ./internal/memory/... ./internal/indexgeneration/... ./internal/api/... pass

The only excluded test is TestWorkspaceImportSpoolsSecureTemporaryFile (asserts POSIX 0600; Windows cannot express that mode and it fails identically on clean main).

Not claimed

No live PostgreSQL, no production-latency run. TestMigrationUpgradeSequence still needs a fresh owned database and was not executed here - rerunning migration/upgrade and PostgreSQL integration checks on the updated head is still required before re-review.

[Bindy-lbb]

Fresh final review on exact head 8263a012e905e1bcff32608538438cd18b5857d3 after full integration of #237. APPROVED: both findings from my previous reviews are resolved, with no remaining blocking finding.

The complete fix branch was fast-forwarded into codex/fix-pr-185, preserving main ancestry and d7a16a7; #237 is recorded as merged. Migration 0027 remains main's vault migration, and hygiene is a byte-identical rename to 0028. Runner assertions, fixture references, upgrade-sequence gates and docs now agree on head 28. scripts/verify.sh is mode 100755 with LF line endings.

Review coverage: all 15 paths in the final PR delta are accounted for across the original review and the six-file integration re-review (15 reviewed, 0 skipped, 100%). Existing cursor ordering/filter binding and batched generation listings retain the original reviewed behavior; no unrelated upstream changes remain in the final delta against main.

Validation on this exact head:

• Local: pinned Goose v3.22.1 collects 28 migrations, and db/memory/indexgeneration/api package tests pass.
• #195's newly triggered CI run 38701995887 succeeds, including Go race/coverage.
• #195's newly triggered Validate Agent memory run 38701995948 succeeds; both Migration and PostgreSQL regression and PostgreSQL race regression explicitly pass.
• All current hosted checks are completed SUCCESS, including Security Baseline and PR Policy.

The live PostgreSQL evidence is from CI; I am not claiming a local PostgreSQL or production-latency run. The prior migration-collision thread is resolved. Approved for the repository's configured squash merge of #195; no protection changes or admin bypass.

[PeterGuy326]

Status: ready to merge — blocked only by the last-push approval rule.

Head 8263a012 is fully green: every check run reports SUCCESS (Go, Worker, Web, PostgreSQL integration, HTTP/CLI/MCP lifecycle, Windows audit evidence, npm wrapper matrix, all three CodeQL languages, Dependency review, Conventional title and linked issue). mergeable=true, behind_by=0.

A squash merge attempt returns:

405 New changes require approval from someone other than Bindy-lbb because they were the last pusher.

main sets require_last_push_approval=true, and the last push to this branch came from @Bindy-lbb (the 0028 hygiene migration resequencing plus the head 28 CI fix were merged into this branch), so @Bindy-lbb's approval on 2026-10-15 does not count for this head. One approval from another code owner on the current head is required — @waterbro-8.

Note the earlier approval on 71db9ab6 (2026-09-24) does not carry over: the head has moved eight commits since.