refactor(ingest): extract the shared local ingestion core (#111) - #217
Conversation
|
CI is green. I cannot approve or merge: I am the author. Needs @PeterGuy326 or @Bindy-lbb. Sequencing: land before any |
|
请评审合入（作者是我，不能自批）。
需要 @PeterGuy326 或 @Bindy-lbb 批准。 |
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
REQUEST CHANGES: the current head 74e31a2 is not approvable yet. On macOS 14 arm64 with Go 1.25.9, the PR body V1 command go test ./cmd/mem -run Ingest -count=1 fails in TestIngestQoderPostsAndCheckpoints, TestIngestQoder409ConflictDegradesPerFile, TestIngestQoderLimitCheckpoint, and TestIngestQoderRelativeRootKeepsProjectSplit. The same command passes on the declared base 1b27aac, so this is a PR regression: the canonical /private/var versus /var path identity and checkpoint expectations are inconsistent. Please make the path contract platform-stable, rerun the documented macOS/Linux/Windows checks, and request re-review.
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
APPROVE
独立复核当前 head dfae2c8：
- 原评审指出的 macOS canonical
/private/var与 fixture/var不一致已修复：Qoder fixture 现在通过ingest.CanonicalRoot返回与实现相同的 source/cursor identity；业务实现未被放宽。 - 当前 head 的 required CI 全绿，包含 Go、PostgreSQL integration、HTTP/CLI/MCP lifecycle、跨 Node 版本 wrapper、Windows audit、CodeQL 与安全基线；Go job 已通过 ingest 回归套件。
- 已核对当前 diff、测试调用路径和
git diff --check结论，没有新增 blocking finding。
LGTM。
dfae2c8 to
70bce9c
Compare
`put --watch` (#110) would otherwise re-implement the same cursor store, state-root layout, failure classification and report vocabulary that PR drift. Move walk, cursor, change gate, `--dry-run` / `--limit` semantics, per-file degradation and report aggregation into `server/internal/ingest` and leave the connector as a thin call site that supplies the Qoder parser, the memory payload and the HTTP upload. Behaviour is preserved at the bytes level, not just by assertion: memories request bodies, `Idempotency-Key` derivation, stdout summary, stderr conflict warning, exit status and cursor file format and location are identical before and after extraction on a shared fixture tree. The PR #108 test suite passes with import-path changes only. Refs #111
The connector used --root exactly as spelled, so the walk, the project/session split and the cursor key disagreed whenever the root was relative or reached through a symlink. Two working directories each holding sessions/p.jsonl shared one cursor, and the second run saw an up-to-date checkpoint and posted nothing. Canonicalize the root once and derive every identity from it. Checkpoint saves staged through one shared <cursor>.tmp, so a second run could fail on the name or rewind a cursor a faster run had already committed. Each save now gets its own staging file and never moves a cursor backwards. Failures now reach the classifier intact: the upload adapter keeps the typed API error and the command maps exit codes at its own boundary, and the transport check no longer runs ahead of the local-file checks that a syscall.Errno also satisfies, so an unreadable source reports read_denied or root_missing instead of network. A cycle that aborts while reading records the code it died on.
Add per-cursor advisory lock (flock on Unix, fcntl on AIX, LockFileEx on Windows) that covers the read/merge/write sequence in SaveCursor. This prevents concurrent ingest processes from regressing a cursor's LastLine or colliding on a shared staging path. The lock sidecar (<cursor>.json.lock) remains on disk after release; the OS releases the advisory lock when the descriptor or owning process exits. Refs #111 Refs #139
70bce9c to
2041c3c
Compare
PeterGuy326
left a comment
PeterGuy326
left a comment
There was a problem hiding this comment.
Reviewed current head 2041c3c. Required CI is green, including Go and PostgreSQL integration; the ingest fixture now uses the same canonical path normalization as production on macOS. No remaining blocker found.
sun-970
left a comment
sun-970
left a comment
There was a problem hiding this comment.
Review — Approve
Scope: #111 extract the shared local ingestion core.
Analysis
-
Canonical path identity ✅ —
ingest.CanonicalRootresolves symlinks and relative paths to absolute canonical paths. Qoder fixture now uses the same normalization as production, fixing the macOS/private/varvs/varinconsistency that PeterGuy326 flagged. -
Cursor locking ✅ — New
cursorLockprovides advisory locking on cursor sidecars with platform-specific implementations (Unix, Windows, AIX). Lock file deliberately persists after release to avoid inode races. -
Backward compatibility ✅ — Cursor file format and location under
~/.mem/ingest/qoderunchanged for roots already given as canonical absolute paths. Relative or symlinked roots get new cursor keys (documented behavior change). -
Test coverage ✅ — Ingest tests cover upload errors, read failures, exit code mapping, relative root checkpoint separation, and project split. PeterGuy326 verified CI green on the latest head including macOS ingest regression suite.
-
CI ✅ — Green across all required checks.
No blocking issues found.
Bindy-lbb
left a comment
Bindy-lbb
left a comment
There was a problem hiding this comment.
Independently reviewed current head 2041c3c. Checked the shared ingest core, Qoder connector, canonical-root identity, cursor persistence/locking across supported OS paths, error classification, and the added regression coverage. git diff --check is clean and all reported required CI checks are green, including Go and PostgreSQL integration. No blocking findings.
5 participants
Refs #111
Requirement and scope
Re-lands #147 onto current
mainas an organization branch. #129/#147 were closed under the 2026-09-03 fork-workflow decision, not as a judgment that the extraction was wrong. Blocker PR #108 is already merged.Preserves qoder behaviour: same memories payload shape, same
Idempotency-Keyderivation for a canonical absolute root, same stdout summary, same cursor file format/location. Adds the OS-backed cursor lock from the #147 follow-up so concurrent writers do not share a.tmpname.Changes
server/internal/ingestpackage: walk, per-path cursor (atomic rename, shrink-reset),--dry-run/--limit, closed failure codes, report aggregation.mem ingest qoderis a thin connector (parser + HTTP upload).cursor_lock_*.go).fsnotify, no--watch(feat(sync): put --watch one-way directory watch (minimal tier, carved from Phase 2 sync drive) #110 stays a successor).Validation ledger
go test ./cmd/mem -run Ingestgo test ./internal/ingestgit diff --checkserver/internal/ingestIndependent review still required. No merge or issue close.
Original extraction: @waterbro-8. Cursor lock follow-up: @sun-970 / liyuanyang. Canonical-path identity follow-up: 勒布朗-詹姆斯.