Skip to content

feat(memory): 统一 L2 提取与修订,兼容后续 Skill 提取 - #798

Open
SeeSeng wants to merge 4 commits into
MemTensor:v1.2.7from
SeeSeng:l2-dev
Open

SeeSeng wants to merge 4 commits into
MemTensor:v1.2.7from
SeeSeng:l2-dev

Conversation

@SeeSeng

@SeeSeng SeeSeng commented Oct 30, 2026

Copy link
Copy Markdown

修改内容

统一 L2 经验提取与更新流程,默认启用新版链路;对后续从 L2 提取 Skill 的提示词、来源追踪及失败恢复做初步兼容。保留旧版 L2 模式,Episode/Trace 直接提取 Skill 的路径不变。

L2 统一提取与自动修订

  • 保留普通 L1、反馈和负向经验三类准入入口,准入后汇入统一候选池,由同一提取流程处理;algorithm.l2Induction.mode 默认使用 unified,可显式切换为 legacy 使用原有链路。
  • 新版以真实 L1 为依据,按批次组织证据,并通过向量召回相关历史 L2;替代新版流程中的 signature 分组,限制单批 L1 数量及上下文长度。
  • L2 分为 principle、local_procedure、check、troubleshooting 四类,分别表达原则、局部流程、检查项和排障经验。
  • 提取模型统一输出 support、revise、create、ignore。将冲突处理收敛为 revise:生成完整新正文,通过来源、目标版本及内容校验后自动应用。
  • 修订前准备新向量,提交时校验内容版本;支持证据按正文版本隔离,避免旧正文的支持记录继续被计入新版。来源撤回后同步使相关派生内容失效。
  • 模型或 embedding 调用失败时保留待处理状态,通过持久化任务重试;反馈降级结果保留后续模型处理机会,避免失败被当作已完成。

L2 → Skill 初步兼容

  • 更新 Skill 提取提示词,使其理解新版四类 L2,并结合对应 L1 证据生成可复用的操作流程。
  • L2 修订后,暂停依赖旧正文的 Skill,并基于当前 L2 重新生成或更新;增加来源版本检查及失败重试,避免继续保留已失效步骤。
  • 当前仍以单条 L2 及其关联 L1 为一次提取输入,并沿用已有 Skill 的匹配与合并机制;本次未新增多条 L2 联合编排 Skill 的能力。
  • Episode/Trace 直接提取 Skill 的入口及流程保持不变。

Embedding 复用与 v1.2.7 兼容

  • 相同文本在同批次去重;L2 查询向量持久化缓存,并在修订与后台处理之间复用。缓存区分完整文本、用途及模型配置,校验向量有效性。
  • 保持 embedding 输入文本、召回阈值及 top-K 不变,不通过缩短正文或减少候选来降低开销。
  • 合入目标分支 v1.2.7,保留其事件循环让出及持久化重试退避;为两个 L2 任务入口补齐 evolution 与 embedding 资源准入,并升级任务策略版本,使已有排队任务使用新策略。

验证结果(2026-10-30,本次提交)

  • Memory 全量测试:3,073 项通过,2 项跳过,0 失败;共 275 个测试文件,273 个通过、2 个跳过。
  • Memory TypeScript 检查通过:tsc -p Memory/tsconfig.json --noEmit。
  • 专项覆盖两个 L2 入口的真实 Worker 资源准入、L2 修订及关联 Skill 恢复、调用失败重试、向量缓存隔离与一致性、召回排序检查。
  • 完成 macOS Apple Silicon 中国版 unsigned 客户端打包,DMG 校验通过;隔离启动后登录界面正常、Memory 健康检查通过。
  • 使用安装包内代码、Electron 和 SQLite 验证默认新版链路、L2 创建与修订、修订回调及持久化查询缓存;另验证安装包内本地 embedding 模型生成有效的 384 维向量,重复输入结果一致。

验证范围

  • 本次基于提交 66ec4616,目标分支为 MemTensor/memmy-agent:v1.2.7。客户端包沿用项目当前版本号 1.2.4,未在本 PR 中调整产品版本号。
  • 自动化故障及安装包 L2 冒烟使用可控模型替身;安装包本地 embedding 单独使用真实本地模型验证。未验证安装包内账号登录及远程模型会话,未执行已有安装的数据升级、Windows/x64 或在线 PolarDB 集成验收。
  • 打包过程中出现既有 DMG 背景替换脚本的镜像空间不足警告,可能影响安装器背景显示;原 DMG 校验及客户端启动通过。该脚本与目标分支一致,本次未修改。
  • 本地评估数据、测试环境适配文件及安装包未纳入提交。

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #798
Task: 8651b9614e7cb3dc
Base: v1.2.7
Head: l2-dev

🔍 OpenCodeReview found 27 issue(s) in this PR.

⚠️ Review warnings (13)

  • completed primary review; remaining group budget is insufficient for another measured pass and finalization
  • completed primary review; remaining group budget is insufficient for another measured pass and finalization
  • primary pass completed; optional refinement did not complete: bounded analysis window ended; explicit finalization required; the grace round did not complete the task
  • primary pass completed; optional refinement did not complete: finalization did not produce an explicit successful task_done for every group file
  • completed primary review; remaining group budget is insufficient for another measured pass and finalization
  • primary pass completed; optional round 2 stopped: analysis_window
  • completed primary review; remaining group budget is insufficient for another measured pass and finalization
  • bounded analysis finalized with explicit task_done after reviewing all group files
  • bounded analysis finalized with explicit task_done after reviewing all group files
  • tool output exceeded the byte/line budget; use narrower file ranges or paginated searches; omitted output is not reviewed
  • … 3 additional warning(s) omitted.

1. Memory/src/storage/repositories.ts (L5226)

invalidateUnifiedL2Source scopes on l2.extract.snapshot.v1:% (and l2.extract.operation.v1:% below), but the unified pipeline only ever writes l2.extract.snapshot.v2:... (l2-extraction-pipeline.ts:304) and l2.extract.operation.v2:... (l2-extraction-pipeline.ts:415). As a result this purge will match zero rows for real data: when an L1 is deleted/withdrawn the v2 snapshots (which persist the full input.L1_RECORDS, i.e. user text) and operation tombstones are never scrubbed. The unit test seeds v1 keys itself, so it does not catch the mismatch. Align both LIKE prefixes with the actual v2 keys (and update the test fixtures accordingly).

💡 Suggested Change

Before:

         WHERE key LIKE 'l2.extract.snapshot.v1:%'

After:

         WHERE key LIKE 'l2.extract.snapshot.v2:%'

2. Memory/src/service/evolution/l2-extraction-pipeline.ts (L432)

This if is inside the else if (operation.operation === "support") branch, so the condition is always true (dead branch). Drop the redundant guard.

💡 Suggested Change

Before:

      if (operation.operation === "support") this.addSupport(target, evidence, at);

After:

      this.addSupport(target, evidence, at);

3. Memory/src/service/evolution/l2-extraction-pipeline.ts (L505-L506)

This inline render duplicates renderL2Lesson exported from l2-content-revision.ts (used by revisedL2Memory). Reuse the shared helper so create/revise stay in sync; otherwise a change to the rendered format must be applied in two places.

💡 Suggested Change

Before:

      value: [`# ${lesson.title}`, `Applicability: ${lesson.applicability}`, `Content: ${lesson.content}`,
        `Verification: ${lesson.verification}`, `Exclusions: ${lesson.exclusions}`].join("\n\n"),

After:

      value: renderL2Lesson(lesson),

4. Memory/src/service/evolution/l2-extraction-pipeline.ts (L499)

Nested ternary (negative ? ... : ordinaryOnly ? ... : ...) is prohibited by the review checklist; move the decision into a small helper or precomputed const for readability. The same pattern is used on the next line for experience_type.


5. Memory/src/service/evolution/l2-extraction-pipeline.ts (L578-L582)

This is a nested ternary (confirmed ? ... : fullyReviewed ? ... : ...), which the checklist disallows. Consider computing retire/fields with an explicit if/else chain or a helper function.


6. Memory/src/service/evolution/l2-extraction-contract.ts (L37-L40)

This chained conditional expression is a multi-level nested ternary, which is disallowed by the review checklist and hurts readability. Prefer a switch (action) (or a lookup map of allowed key sets) to make the per-operation contract explicit.


7. Memory/src/algorithm/plugin-algorithms.ts (L1522-L1524)

The five-line "L2 compatibility" contract is duplicated verbatim in L2_SKILL_CRYSTALLIZE_PROMPT and SKILL_REBUILD_PROMPT, so a later edit to one variant will silently diverge from the other. Extract it into a shared constant (e.g. const L2_COMPATIBILITY_RULES = "...") and concatenate it in both prompts. Also note this block is appended to the crystallize prompt even though EXISTING_SKILL_SNAPSHOT is only supplied on the rebuild path (see skill-pipeline.enhanceSkillDraft), so that reference is unreachable in the crystallize contract.


8. Memory/src/algorithm/plugin-algorithms.ts (L3371)

numberField(policy, "schema_version") is computed three times inside this object literal, which makes the v2 branch easy to miss when editing defaults (experienceType, evidencePolarity, skillEligible). Hoist it once at the top of the function, e.g. const schemaVersion = numberField(policy, "schema_version");, and reuse it, so the v2 fallbacks stay in one obvious place.


9. Memory/src/service/embedding/embedding-job-processor.ts (L305)

The cache key is derived from a hardcoded "query" literal while applyEmbeddingVector re-derives it from item.role. Today both are "query" so the guard always passes, but if the L2 role is ever changed the write-side guard would silently become false and the cache would never be populated (reads keep using the query namespace). Derive the key from the same value that is stored on the item (e.g. build the item first with role, then set l2VectorCacheKey from item.role) so the two sides cannot drift.


10. Memory/src/service/embedding/embedding-job-processor.ts (L327)

The cached vector is keyed by item.text, but the embedding is computed by the callers on item.text || "(empty)" (runLeasedEmbeddingJobs/runLeasedEmbeddingItem). For an L2 row with empty text this stores a vector for the literal "(empty)" under the empty-string key, and any later empty-text L2 row (or the L2 extraction pipeline, which does not substitute) would reuse that unrelated vector. Skip the cache write when the text is empty.


11. Memory/src/service/embedding/l2-vector-cache.ts (L15)

The persistent L2 vector cache ignores config.cache (EmbeddingConfig.cache), which already disables the embedder's in-memory cache in model/embedder.ts. With cache: false vectors are still persisted to runtime_kv keyed by the text digest, so the opt-out is not honoured. Additionally, entries are never expired or pruned, and every L2 content revision produces a new key, so runtime_kv grows without bound. Please gate l2VectorCacheKey/readL2VectorCache on config.cache and provide a retention/pruning strategy for the l2.extract.vector.v1: prefix.


12. Memory/src/service/evolution/reward-pipeline.ts (L313)

recomputePolicyStats is not a pure read: it writes the L2 row, appends a policy_stats_update change, calls onSkillRewardDrift (which mutates dependent skill eta/status) and enqueues skill_crystallization without a dedupeKey. Calling it once per updated trace means a policy shared by N traces in the same backprop is recomputed N times, producing redundant writes/change rows and N near-duplicate crystallization jobs. Collect the affected policy ids across the loop and recompute each once after the loop.


13. Memory/src/service/evolution/skill-pipeline.ts (L434-L436)

enhanceSkillDraft now returns llm-failed: configured skill model unavailable when the model is not configured, and this branch throws for every llm-failed: reason. Previously an unconfigured skill model produced the benign llm_disabled skip and the job completed; now every crystallization/revision job fails, burns its retry attempts, and (because the throw happens inside the policyMemories loop) also aborts the remaining policies in that job. A misconfiguration should not be treated as a transient retryable LLM failure — return a non-llm-failed skip reason (e.g. skill_model_unavailable) and continue.


14. Memory/src/service/evolution/skill-pipeline.ts (L284-L286)

policyMetaFromMemory(current) can return null (e.g. the policy payload was rewritten/invalidated concurrently); the ! assertion would then throw a raw TypeError: Cannot read properties of null from inside the write transaction instead of the intended retryable "source changed" error. Guard the parsed metadata explicitly and throw the retry error when it is missing.

💡 Suggested Change

Before:

      const current = this.deps.repos.memories.get(policy.id);
      if (!current || current.deletedAt || current.status === "archived" || current.version !== source!.version
        || skillPolicyContentHash(policyMetaFromMemory(current)!) !== skillPolicyContentHash(policy)) {

After:

      const current = this.deps.repos.memories.get(policy.id);
      const currentMeta = current ? policyMetaFromMemory(current) : null;
      if (!current || !currentMeta || current.deletedAt || current.status === "archived" || current.version !== source!.version
        || skillPolicyContentHash(currentMeta) !== skillPolicyContentHash(policy)) {

15. Memory/src/service/evolution/skill-pipeline.ts (L289)

After a successful revision the pending_policy_revisions[policy.id] entry is written by suspendForPolicyRevision but never cleared here (only policy_content_hashes is updated), so the skill keeps a stale pending revision forever. The crystallization gate later re-reads that map for every policy id, and any future content change on that policy silently re-enters the pending state without a new suspension/enqueue. Clear the satisfied entry as part of the commit fields.

💡 Suggested Change

Before:

      const hashes = { ...storedSkillPolicyContentHashes(row), [policy.id]: skillPolicyContentHash(policy) };

After:

      const hashes = { ...storedSkillPolicyContentHashes(row), [policy.id]: skillPolicyContentHash(policy) };
      const { [policy.id]: _resolvedPending, ...remainingPending } = pending;

16. Memory/src/service/evolution/l2-extraction-pipeline.ts (L201-L202)

In the failure path, persistFeedbackFallbacks runs renew() inside its own transaction. If the scope lease was taken over (or expired) renew() throws, so the fallback-persistence error replaces the original extraction error and the real cause is lost from logs/retry diagnostics. Wrap the fallback write so the primary error is always the one rethrown.

💡 Suggested Change

Before:

      this.persistFeedbackFallbacks(candidates, renew);
      throw error;

After:

      try {
        this.persistFeedbackFallbacks(candidates, renew);
      } catch (fallbackError) {
        logEvolutionDecision(job, "l2_induction", "unified_fallback_persist_failed", { error: String(fallbackError) });
      }
      throw error;

17. Memory/src/service/evolution/l2-extraction-pipeline.ts (L532-L533)

listL2Candidates is called once per candidate inside this loop, and each call re-loads and re-upserts every pending candidate for the episode (each upsert opening its own nested transaction and re-reading the source L1). That makes this loop O(n²) DB work inside a single transaction. Fetch the pending list once before the loop and look up each candidate by id/version instead.

💡 Suggested Change

Before:

        const pending = listL2Candidates(this.deps.repos, { userId: candidate.userId, episodeId: candidate.episodeId })
          .find((item) => item.id === candidate.id && item.sourceVersion === candidate.sourceVersion);

After:

        const pending = listL2Candidates(this.deps.repos, { userId: candidate.userId, episodeId: candidate.episodeId })
          .find((item) => item.id === candidate.id && item.sourceVersion === candidate.sourceVersion); // hoist this list outside the loop

18. Memory/src/service/evolution/l2-extraction-pipeline.ts (L308-L309)

The pending snapshot is written before the model call and stores the complete input (all L1 records plus historical candidates). If extraction keeps failing (transport/validation errors), these rows stay in runtime_kv forever with a full copy of the source text — nothing ever prunes pending snapshots (only the committed one is reused). Consider deleting/truncating the pending snapshot when the batch aborts, or add a retention sweep for non-committed l2.extract.snapshot.* entries.


19. Memory/src/service/evolution/l2-candidate-service.ts (L124-L128)

This refresh relies on the implicit invariant that upsertL2Candidate merges the argument route into the previously stored routes. Passing evidence.routes[0] (an arbitrary member of the already-stored list) only works because nothing drops the other routes; if the merge logic ever validates or filters routes/signals, a plain refresh would silently truncate provenance. A dedicated refreshL2CandidateSource(repos, { sourceMemoryId, userId }) (no route argument) would make the intent explicit and remove the dependency.


20. Memory/src/service/evolution/l2-content-revision.ts (L27)

The field set built here has drifted from the one used by createLesson in l2-extraction-pipeline.ts: this version also writes title/trigger/procedure/verification/boundary, decision_guidance, expected_outcome and exclusions into info/internal_info, while createLesson only stores them under policy. Consumers reading memory.info.title will therefore see different shapes for a created vs. a revised lesson. Extract a single shared field builder for create/revise so both produce the same schema.


21. Memory/src/service/memory-service.ts (L2978)

This per-page list() + traceMeta() scan is heavier than needed: traceMeta() issues a getRawTurn() query and JSON parsing for every L1 memory in the session, so redacting one raw turn costs O(session L1 size) DB round-trips. this.repos.runtime.l1MemoryIdsForRawTurn(rawTurn.id) already resolves the L1 ids for a raw turn (info/internal_info.trace paths); combining it with a single targeted lookup for internal_info.source_raw_turn_id and memories.getMany() would avoid the full scan. Also note that offset pagination is being used while invalidateMemoryDependencies() mutates dependent rows — if any touched row falls into the created_at DESC, updated_at DESC, id DESC ordering, later pages can skip entries.


22. Memory/src/model/llm.ts (L254-L255)

This early throw bypasses the failure bookkeeping used by every other exit path of completeJson: this.lastError is not updated and no json.failed event is logged, so status().lastError stays stale and the truncation is invisible in diagnostics. Consider reusing the shared failure path (set this.lastError, log with memoryErrorFields(...), include operation/attempt/finishReason) or at least log a json.failed entry before throwing.


23. Memory/src/config/index.ts (L1286-L1287)

recallTopK is floored while maxInputChars/maxOutputTokens are only floored at the lower bound without being made integral (e.g. 48000.7 would be stored as-is and compared against input.length). Suggest normalizing all counts consistently with Math.floor(...) and, for maxOutputTokens, keeping it integral since it is passed straight to the provider as maxTokens.


24. Memory/src/service/feedback/feedback-experience.ts (L2931-L2933)

The literal "Continue using this approach:" is duplicated here and in refineFeedbackExperienceByRules (line ~2819) where it is concatenated into procedure. If that wording ever changes, this check silently stops matching and raw feedback text is no longer substituted (or, conversely, a non-praise procedure would be replaced by raw text). Extract the phrase into a shared const and build the procedure from it in both places.

💡 Suggested Change

Before:

  const content = refinement.procedure.startsWith("Continue using this approach:")
    ? input.text
    : refinement.procedure;

After:

  const content = refinement.procedure.startsWith(FEEDBACK_CONTINUE_APPROACH_PREFIX)
    ? input.text
    : refinement.procedure;

25. Memory/src/service/feedback/feedback-experience.ts (L2920-L2922)

Nested ternary, which the project style rules disallow. Replace it with an if/else chain or a small helper so the precedence (check > principle > local_procedure) stays readable.

💡 Suggested Change

Before:

  const type = hasCheck ? "check"
    : shape === "preference" || shape === "constraint" ? "principle"
    : "local_procedure";

After:

  let type: "principle" | "local_procedure" | "check" = "local_procedure";
  if (hasCheck) type = "check";
  else if (shape === "preference" || shape === "constraint") type = "principle";

26. Memory/src/service/feedback/feedback-experience.ts (L819)

traceMemory is only narrowed inside the ternary above, so the non-null assertion is needed here even though candidate !== null implies it. Restructure to keep the narrowing (e.g. early const source = traceMemory && trace && traceMemory.userId === feedback.userId ? traceMemory : undefined; then if (!source) {...}, and read source.sessionId), so the invariant is enforced by types rather than by !.


27. Memory/src/service/trials/skill-trial-resolver.ts (L153)

Timestamps are compared as raw strings (also in the item.createdAt > revisedAt filter below), which only holds when both values use the exact same ISO-8601 form. content_revised_at comes from the at argument of suspendForPolicyRevision, so any caller passing a differently formatted/precision timestamp (or a row written by an older version) will silently include or exclude historical trials and skew attempted/passed. Compare parsed instants instead.

💡 Suggested Change

Before:

    if (revisedAt && trial.createdAt <= revisedAt) return;

After:

    const revisedMs = revisedAt ? Date.parse(revisedAt) : NaN;
    // Historical trials remain auditable, but cannot validate rewritten content.
    if (Number.isFinite(revisedMs) && Date.parse(trial.createdAt) <= revisedMs) return;

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: NO TEST SCOPE

Automated tests were not run because the changed files do not map to an executable test scope.

Details: No executable test scope maps to the changed files. Automated tests were not run; add env.yaml source_mapping + execution for this path family, then rerun. Changed files: Memory/src/algorithm/plugin-algorithms.ts, Memory/src/config/index.ts, Memory/src/model/llm.ts, Memory/src/model/types.ts, Memory/src/service/embedding/embedding-job-processor.ts, Memory/src/service/embedding/l2-vector-cache.ts, Memory/src/service/evolution/evolution-job-processor.ts, Memory/src/service/evolution/l2-candidate-service.ts, Memory/src/service/evolution/l2-content-revision.ts, Memory/src/service/evolution/l2-extraction-contract.ts
Manual review or env.yaml source_mapping/execution coverage is required before merge.

Branch: l2-dev

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants