Skip to content

chore(ingest): extract the shared local ingestion core (walk, cursor, change gate, report) from the qoder connector #111

Closed

Description

@waterbro-8
schemaVersion: requirement-record.v1
revision: R1
status: needs-design
priority: P2
productOwner: "@PeterGuy326"
technicalOwner: "@waterbro-8"
userOutcome: "A mem maintainer changes local ingestion behaviour once, in one core, instead of twice: the qoder transcript connector and the future put --watch daemon share walk, cursor, change-detection and report code, so #110 does not write a second state layer."
requirements:
  - REQ-001
  - REQ-002
  - REQ-003
acceptanceCriteria:
  - AC-001
  - AC-002
  - AC-003
parent: null
dependencies:
  - "https://github.com/bytefolk/mem/pull/108 (must merge first)"
supersedes: []
lastDecisionAt: null

User problem and observable outcome

Adjudicated on #110 (R2, D-4): #110 and #103 share an ingestion core, not a write surface. The core already exists in working form inside #103's connector, and everything in it was earned through review on PR #108 — a cursor that must not advance under --dry-run, truncate/rewrite detection, atomic cursor writes, per-file degradation on a 409 instead of aborting the run, and a run summary.

Concrete duplication cost if this is not done: put --watch would re-implement the same cursor store, the same state-root layout (cliStateRoot(), server/cmd/mem/cmds_ingest.go:104-112), the same failure-classification table and the same report vocabulary, in a second file, with a second set of edge-case tests, and the two copies would drift. Observable outcome: one internal package owns local ingestion mechanics; both call sites are thin.

Requirements

  • REQ-001: Extract the local ingestion core — deterministic recursive walk, per-path cursor store (atomic rename write, truncate/rewrite reset), stat-gate-then-hash change decision, cycle report aggregation, closed failure-code classification, and the --dry-run / --limit semantics — out of server/cmd/mem/cmds_ingest.go + qoder_checkpoint.go into a reusable internal package, preserving behaviour exactly.
  • REQ-002: Point the qoder connector at the core with no observable behaviour change: same memories payload shape, same Idempotency-Key derivation, same stdout summary, same cursor file format and location (existing users' cursors must remain readable).
  • REQ-003: Make the report and failure vocabulary explicit and shared, so feat(sync): put --watch one-way directory watch (minimal tier, carved from Phase 2 sync drive) #110 consumes names instead of inventing them: counts scanned / ingested / deduped / unchanged / changed / local_gone / failed, and failure codes auth / plan_quota / provider_timeout / network / read_denied / upload_rejected / root_missing / state_corrupt.

Acceptance criteria

Non-goals and forbidden shortcuts

Lifecycle, status, priority, blockers, and open decisions

Evidence plan

Decisions

Revision history


需要 maintainer 打标(本账号无 label 写权限):建议 type:maintenance、area:cli、priority:p2、evidence:e2-source、status:needs-triage。

Activity

  1. PeterGuy326 commented on Sep 1, 2026

    @PeterGuy326
    Collaborator
    schemaVersion: requirement-decision.v1
    canonicalIssue: "https://github.com/fullstack-ai-infra/mem/issues/111"
    previousRevision: R1
    resultingRevision: R2
    resultingStatus: ready
    decisionType: narrowing-and-dependency-gate
    approver: "@PeterGuy326"
    decidedAt: "2026-09-01T02:54:08Z"
    dependency: "https://github.com/fullstack-ai-infra/mem/pull/140 accepted and merged"

    R2 owner decision — strict mechanical ingest extraction

    This is the effective product-owner decision for R2. The contract is ready, but implementation is prohibited until #140 has been independently accepted and merged. It is not a PR approval or merge authorization.

    Approved scope

    • Extract only the already observable qoder-ingestion mechanics into server/internal/ingest; the work is a mechanical refactor, not a behavior change.
    • Preserve the existing request bodies, cursor bytes and locations, report/output behavior, dry-run handling, shrunk-or-rewritten-file handling, per-file 409 degradation, and corrupt-cursor behavior.
    • Carry forward the independent concurrent-checkpoint max-progress safeguard from fix(ingest): serialize qoder checkpoint writers #140.

    Acceptance evidence

    • On the same deterministic fixture, a byte-level pre/post comparison proves identical request payloads, cursor artifacts, and command output.
    • Focused tests prove dry-run makes neither request nor cursor write; a shrunk/rewritten file resumes under the existing semantics; a 409 degrades one file without stopping the run; and a corrupt cursor degrades under the existing failure semantics.
    • The package has no Cobra or stdout dependency and introduces no third-party dependency.

    Non-goals

    No new content-hash or change gate, path rekeying or replay behavior, change to payloads, failure codes, or report vocabulary, watch daemon, fsnotify, or semantic parser improvement is authorized. Any such behavior change requires a separate owner decision.

    After #140 is accepted and merged, implementation may be planned against R2 with normal independent review, required CI, and merge evidence still required.

  2. sun-970 commented on Sep 1, 2026

    @sun-970
    Contributor

    Implementation: PR #147 (supersedes #129, rebased onto latest main).

    Changes:

    • Extracted walk, cursor, change gate, report, and failure classification to server/internal/ingest
    • Added OS-backed cursor lock (flock/fcntl/LockFileEx) for concurrent-writer safety per R2 decision
    • All existing tests pass; new tests pin edge cases

    Head SHA: 785de61e00661f4e5115129500be642c47422108

  3. waterbro-8 commented on Sep 2, 2026

    @waterbro-8
    CollaboratorAuthor

    Design decision requested — I am the technicalOwner here, and I am not taking it unilaterally. Disclosure, because it matters for how to read this: two of #147's three commits (e9e17f8b, ab5bc035) are authored by this account, and #129 (the draft this one supersedes) is ours too. So the ruling below is @PeterGuy326's as productOwner, not ours to make.

    Status vs reality

    The requirement record is still status: needs-design, and it says "Do not implement this before PR #108 merges" — #108 merged 2026-08-30T06:14:00Z, so that blocker is genuinely cleared. But an implementation is open now as #147 (fork sun-970/mem, head 785de61e), carrying our two extraction commits plus a third that lands #139's lock inside the same PR. #129, our own draft of the same work, sits DRAFT + CHANGES_REQUESTED. Someone should decide whether #111 moves to in-design on the strength of #147 or whether the sequencing gets reset; either is defensible, "neither" is the current state.

    The open decision got exercised without a ruling

    "Open decisions for the owner at kickoff" lists: whether cursors key on absolute path or on a path-plus-device id (symlinked roots). #147 answers it by adding ingest.CanonicalRoot with filepath.EvalSymlinks (server/internal/ingest/ingest.go:196-205) and routing the root through it before anything is keyed. That resolves the collision bug it cites — two cwd's each holding sessions/p.jsonl do now get distinct cursors — and the collision fix needs only filepath.Abs. The EvalSymlinks step is what has a user-visible cost:

    • the cursor filename is sha1(abs), so a root spelled relatively or reached through a symlink loses its existing cursors;
    • abs also feeds ingestIdempotencyKey(abs, line) (server/cmd/mem/cmds_ingest.go:317-320), so every per-line key changes for that root;
    • the server replays on idempotency key alone — internal/memory/service.go:107 is ON CONFLICT (workspace_id, idempotency_key_sha256) DO NOTHING, the replay lookup at :141-148 selects on the same pair, and no migration adds content-, path- or source.ref-based uniqueness. A new key is therefore a new row: Replayed: false.

    So the re-post is not a replay. For a store behind a symlink (a dotfile manager symlinking ~/.qoder is the ordinary case) one upgrade pass duplicates every memory in it. #147's docs/integrations/qoder-ingest.md and CHANGELOG both describe this as "re-posted once under new keys" / "replays those files once", which understates it, and REQ-002's own words are "existing users' cursors must remain readable" — readable, yes; equivalent, no. AC-001 asks for more than that: "a pre/post comparison on one fixture transcript tree yields identical memories bodies and identical cursor bytes", which cannot hold for this class of root. #147's requirements trace restates AC-001 without that clause.

    Three ways out, in rough order of how little they cost users:

    1. Key on the absolute-but-unresolved path (filepath.Abs only). Collision bug fixed, no re-key, no duplicates. Cost: the same store reached via two spellings keeps two cursors — which is a re-ingest saved by idempotency, not a correctness loss.
    2. Keep canonical keys but make it a decision with a migration: read the legacy sha1(spelled) cursor as a fallback on miss, and derive the idempotency key from a stable identity that predates the change.
    3. Accept the re-key, and say in the CHANGELOG that upgrading duplicates that store's memories, so the operator can clear or dedupe deliberately.

    Option 3 is the only one that is strictly a documentation fix, and it is the one I'd not pick: it makes the tool write duplicates as a side effect of a refactor PR.

    Two smaller things from the same record

    No PR opened, nothing merged or closed, no workflow approval, no review submitted on any of #129/#139/#140/#147.

  4. waterbro-8 commented on Sep 3, 2026

    @waterbro-8
    CollaboratorAuthor

    Coverage read-back: this work is orphaned by a handoff that no longer exists

    Three things happened to the shared ingestion core in four days, and the end state is that no vehicle exists for it:

    1. refactor(ingest): extract the shared local ingestion core #129 (ours, opened by waterbro-8, head 6e864e1e) was closed unmerged on 2026-09-02T05:36Z with the reason recorded in-thread: "Closing this as superseded. It is our PR and our draft, and refactor(ingest): extract shared local ingestion core with concurrent-writer safety #147 states 'Supersedes refactor(ingest): extract the shared local ingestion core #129' in its own body."
    2. refactor(ingest): extract shared local ingestion core with concurrent-writer safety #147 was a fork pull request. It was closed on 2026-09-03 under the decision that fork workflow permissions stay off and this work is redone in-repo, and no code was imported.
    3. So the handoff in step 1 pointed at a PR that no longer exists, and the fallback in step 2 has not started. refactor(ingest): extract the shared local ingestion core #129's Supersedes line and refactor(ingest): extract shared local ingestion core with concurrent-writer safety #147's close comment are the only two records of the transfer, which is why this comment exists.

    Measured on main@1332bf46, the extraction never landed: server/internal/ingest/ does not exist, and server/cmd/mem/qoder_checkpoint.go -- the file #147 proposed deleting -- is still there. #147's own file list showed the new package as seven added files under server/internal/ingest/.

    Reopening #129 is not the clean fix it looks like

    The attribution problem does not resolve by falling back to our own PR, because #129's head is itself mixed:

    commit author committer
    3c123cdb refactor(ingest): extract the shared local ingestion core waterbro-8 <waterbro-8@users.noreply.github.com> same
    6e864e1e fix(ingest): give one transcript store one identity 勒布朗-詹姆斯 <2986253039@qq.com> same

    Two separate defects in that table, and neither is cosmetic. 6e864e1e is authored by a personal @qq.com address, which is the shape today's rule excludes. And 3c123cdb's email is waterbro-8@users.noreply.github.com -- not the ID-prefixed 318569545+waterbro-8@users.noreply.github.com form -- so GitHub does not map it to the account; it renders as an unlinked author string, which is what #138's "clean candidate supersedes #132" reasoning was about. #129 also still carries an unresolved CHANGES_REQUESTED from @PeterGuy326 (audit at 3c123cdb, response posted 08-31).

    Re-authoring those commits under one identity would be the other horn of the same problem: it silently rewrites who wrote what. That is a call for the owners of this repository, not something to do quietly in a rework.

    What I did not do

    Did not reopen #129, did not cherry-pick from #147, did not rewrite any commit authorship, did not start the in-repo branch, and changed no label, status, assignment, or body here. The decision recorded today says redo in-repo; #111 is the one item where "redo" needs a prior ruling on whose work the two commits are, because the code exists and the attribution is contested.

    Related: #110 was covered by the same #147 and by #150, and has its own read-back there.

  5. sun-970 commented on Sep 8, 2026

    @sun-970
    Contributor

    Path forward for #111

    The extraction work currently has no vehicle: #129 is closed with attribution concerns, and #147 (my fork PR) was closed under the in-repo workflow decision. Summarising what I believe is needed to unblock:

    1. Attribution ruling (owner call)

    #129's two commits have authorship issues documented by @waterbro-8. I defer to the repo owners on how to resolve this — options are: (a) cherry-pick the code changes onto new clean commits, (b) re-author under corrected identities with explicit acknowledgment, or (c) redo from scratch. I am willing to do (c) if that is the cleanest path.

    2. Symlink cursor key: option 1

    Per @waterbro-8's analysis, filepath.Abs only (without EvalSymlinks) fixes the collision bug without re-keying existing cursors or duplicating memories. I will implement option 1 unless the owners direct otherwise.

    3. Proposed next step

    With owner confirmation on the above two points, I will open a fresh in-repo PR that:

    No scope creep beyond R2's non-goals.

    @PeterGuy326 could you confirm the direction on (1) and (2) so this can move?

  6. PeterGuy326 commented on Sep 10, 2026

    @PeterGuy326
    Collaborator

    Initial triage — labels applied

    Per docs/maintainers/triage.md §"Label taxonomy" ("During initial triage, a mem maintainer applies the type, status, area, evidence, and—when applicable—severity labels"), this issue carried no labels at all, so it was invisible to every label-based view. Applied the set the author requested at the foot of the body, which matches the taxonomy:

    type:maintenance (refactoring, no user-visible behaviour change) · area:cli (both sinks are CLI-driven; the issue itself rules out a server-side home) · evidence:e2-source (the duplication is locatable at server/cmd/mem/qoder_checkpoint.go and cmds_ingest.go; nothing was executed) · status:needs-triage.

    Not applied: severity: — maintenance issues do not normally carry one. Not raised: evidence: stays at E2; E3 requires independent reproduction, which is a delivery-time check here (identical cursor bytes and memories bodies before/after).

    The named blocker is cleared

    The record says: "Blocker: merge of PR #108". PR #108 is MERGED — 365ff09b2327, 2026-08-30T06:14:00Z, feat(ingest): add mem ingest qoder for AI-agent conversation transcripts (#103). So the precondition for starting this work is satisfied and the issue is no longer blocked.

    Current state on main (verified at 55d09b203ba9, 2026-09-10T16:23:43Z)

    Not started. grep -rn 'package ingest' server/ returns nothing; there is no server/internal/ingest. server/cmd/mem/qoder_checkpoint.go (89 lines) still owns qoderCheckpointPath / loadQoderCheckpoint / saveQoderCheckpoint / fileState locally, and git log --follow shows its only commit is 365ff09b2327 (#103) — it has never been touched since. cmds_ingest.go is 345 lines. Both prior implementation attempts, #129 and #147, were closed unmerged.

    Readiness-gate note (not a status:ready recommendation)

    Bullets 1–6 and 8–9 pass on the record as written (not a duplicate; no sensitive detail; labels now accurate; clear outcome; AC-001/002/003 are objective and testable; dependencies and risks named; validation approach is the byte-identical before/after comparison). Two bullets do not yet pass:

    • Bullet 7 (scope and explicit non-goals recorded) — non-goals are recorded, but the scope is split across two still-open owner decisions: package location (server/internal/ingest vs an internal file under server/cmd/mem), and whether cursors key on absolute path or path-plus-device id. The issue assigns these to "the owner at kickoff", and technicalOwner is @waterbro-8 while productOwner is @PeterGuy326.
    • Bullet 10 (small enough for one reviewable PR) — plausible but unproven; refactor(ingest): extract shared local ingestion core with concurrent-writer safety #147 attempted it and did not land.

    There is also a sequencing contradiction to resolve before promotion: this record says "Sequencing: this lands before #110's implementation PR", while #110's own record says "Blockers: none". One of the two should be corrected.

    Left at status:needs-triage deliberately — the gate is not stated bullet-by-bullet as passing, so status:ready is not applied here.

  7. added
    type:maintenanceMaintenance, tooling, refactoring, or repository work
    area:cliCommand-line interface
    evidence:e2-sourceSource or log evidence identifies the likely cause
    on Sep 10, 2026
  8. waterbro-8 commented on Sep 17, 2026

    @waterbro-8
    CollaboratorAuthor

    Not implemented on main (server/internal/ingest does not exist). Previous candidates #129 and #147 were closed under the 2026-09-03 fork-workflow decision.

    Blocker PR #108 has merged, so extraction can start, but #147 was explicitly warned as unsafe to reuse blindly (cross-process cursor vs #139). Sequencing: land #191 first, then extract the core onto an organization branch, then #110.

    I am not opening a watch or ingest-core PR ahead of that order.

  9. waterbro-8 commented on Sep 17, 2026

    @waterbro-8
    CollaboratorAuthor

    Organization-branch successor to #147 is up: #217.

    Cherry-picked onto current main (1b27aac): extract server/internal/ingest, keep qoder as a thin connector, and include the OS-backed cursor lock so this does not revive the shared .tmp race that #139/#191 describe.

    #110 can follow this PR. I am not implementing --watch here.

  10. waterbro-8 commented on Sep 20, 2026

    @waterbro-8
    CollaboratorAuthor

    平台组分工补派(冯浩然 / waterbro-8)

  11. waterbro-8 commented on Sep 20, 2026

    @waterbro-8
    CollaboratorAuthor

    平台组群成员派活(冯浩然 / waterbro-8)

  12. waterbro-8 commented on Sep 23, 2026

    @waterbro-8
    CollaboratorAuthor

    关闭验收:PR #217 已合入 main(merge f72e294;final head 2041c3c),server/internal/ingest 已提供共享 walk、cursor/change gate、closed failure codes、report 与 OS-backed lock,Qoder 变为薄连接器。final head 有多份正式 APPROVED,全部 hosted checks 通过;下游 #110 已实际复用该 core 并合入。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:cliCommand-line interfaceevidence:e2-sourceSource or log evidence identifies the likely causestatus:needs-triageAwaiting maintainer classificationtype:maintenanceMaintenance, tooling, refactoring, or repository work

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions