Skip to content

fix(2c): separate policy gain from normalized reward credit - #2372

Open
chiefmojo wants to merge 9 commits into
MemTensor:mainfrom
chiefmojo:pr/wp272-policy-gain-repair
Open

chiefmojo wants to merge 9 commits into
MemTensor:mainfrom
chiefmojo:pr/wp272-policy-gain-repair

Conversation

@chiefmojo

@chiefmojo chiefmojo commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Separates policy gain (used for repair-eligibility/promotion decisions) from normalized reward credit V, and fixes a real promotion stall: with the current scheme, gain is a bare running average with no contributor-count scaling, so once enough negative-evidence traces accumulate, a policy's gain floor-locks below the promotion threshold even when recent evidence is strongly positive — candidates never get a fair look and the pool grows unbounded.

This is a deeper fix than #2364/#2367 (which only retunes MIN_ADAPTIVE_BASELINE, a constant, without changing the underlying scaling). It:

  • Persists a contribution-adjusted gainValue per trace, separate from V, with versioned historical provenance (live_normalized / inferred_normalized / legacy_unscaled / unresolved).
  • Ships an idempotent, bounded historical-inference pass (core/reward/gain-inference.ts) that runs once per boot, before the repair timer/consumers start, so partially-converted state is never observable. Bound is config-tunable (gainInferenceBootMaxGroups / gainInferenceBootTimeBudgetMs) for hosts with a large backlog.
  • Adds a budgeted, timer-driven repair engine (core/memory/l2/gain-repair.ts) that drains the candidate/active backlog on a configurable cadence (gainRepairBatchSize, gainRepairIntervalMs, gainRepairMaxTotal) instead of requiring a bulk one-shot migration.
  • Adds a read-only preview RPC (policies.gainPreview) and a policy-field CAS rollback RPC (policies.gainRollback) for safe operability.
  • Schema-only migration (019-policy-gain-value.sql): adds columns + the repair queue/journal tables. No data conversion, no queue seeding, no policy/trace writes at migration time — all of that happens in the idempotent TS passes above.

All of this is gated behind algorithm.l2Induction.gainV2Enabled (default false) — a blank/upgraded config is a no-op until explicitly turned on.

Burned in on three internal hosts at increasing corpus scale (the largest ~1.8k policy groups / ~28k traces) with zero errors/conflicts/failed across every observed repair tick, and real candidate→active promotions where the fleet had produced none in 16–69 days prior.

Test plan

  • npm run lint (tsc --noEmit) — clean, no new errors
  • npx vitest run — all green across the suites that cover this change (memory/l2, reward, storage, pipeline, config, bridge, agent-contract): 65 files / 611 tests passing. The only files that cannot execute in this environment are adapters/deepseek-harness-*, which need private @deepseek-ai/* packages; they are excluded from the counts rather than reported as failures.
  • Targeted suites for the new code: tests/unit/memory/l2/, tests/unit/reward/, tests/unit/storage/gain-repair-migration.test.ts, tests/unit/pipeline/gain-repair-*.test.ts, tests/unit/bridge/methods.test.ts, tests/unit/agent-contract/gain-maintenance-contract.test.ts, tests/unit/storage/owner-exact.test.ts, tests/unit/config/load.test.ts — all pass

@Memtensor-AI Memtensor-AI added area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Sep 16, 2026
@Memtensor-AI

Memtensor-AI commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2372
Task: 3d3589b921ab9726
Base: main
Head: pr/wp272-policy-gain-repair

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

⚠️ 1 warning(s) occurred during review.


1. apps/memos-local-plugin/core/config/schema.ts (L365)

The schema allows gainInferenceBootTimeBudgetMs up to 300,000 ms (5 minutes), but gain-inference.ts documents that both boot bounds are "deliberately conservative against the 120s watchdog default". A value near the 300,000 ms maximum exceeds the 120,000 ms watchdog default by 2.5×. If an operator raises this field to, say, 250,000 ms on a host where initWatchdogMs remains at its default, the synchronous boot inference pass will hold init past the watchdog, triggering a kill → restart loop.

Suggestion: lower the maximum to a value that is provably safe relative to the watchdog default — e.g., 90,000 ms — or add a cross-field validation against the resolved initWatchdogMs value at startup, with a warning/rejection when gainInferenceBootTimeBudgetMs ≥ initWatchdogMs.

💡 Suggested Change

Before:

    gainInferenceBootTimeBudgetMs: Type.Integer({ default: 30_000, minimum: 1_000, maximum: 300_000 }),

After:

    gainInferenceBootTimeBudgetMs: Type.Integer({ default: 30_000, minimum: 1_000, maximum: 90_000 }),

2. apps/memos-local-plugin/bridge/methods.ts (L394-L400)

When the caller passes a journalIds array whose entries are all non-string or empty strings, the .filter(...) silently discards them all, journalIds becomes a defined-but-empty array, and execution falls through to the final throw with the message "exactly one of 'batchId' / non-empty 'journalIds' is required". That message is misleading: the caller did supply journalIds, but every entry was invalid. A caller seeing this error has no way to know whether they forgot the field entirely or supplied it with malformed values.

Suggestion: distinguish the two cases with separate messages.

💡 Suggested Change

Before:

        if (journalIds !== undefined && journalIds.length > 0) {
          return await core.rollbackGainRepair({ namespace: ns, journalIds });
        }
        throw new MemosError(
          "invalid_argument",
          `${method}: exactly one of 'batchId' / non-empty 'journalIds' is required`,
        );

After:

        if (journalIds !== undefined) {
          if (journalIds.length > 0) {
            return await core.rollbackGainRepair({ namespace: ns, journalIds });
          }
          throw new MemosError(
            "invalid_argument",
            `${method}: 'journalIds' was provided but all entries were invalid (must be non-empty strings)`,
          );
        }
        throw new MemosError(
          "invalid_argument",
          `${method}: exactly one of 'batchId' / non-empty 'journalIds' is required`,
        );

3. apps/memos-local-plugin/bridge/methods.ts (L380-L384)

There is no upper bound on the number of journal IDs accepted at the bridge. A caller can pass an arbitrarily large array; the filtered list is forwarded directly to core.rollbackGainRepair, which in turn passes it to the storage layer as individual getJournalById + updateStats calls per entry inside a single transaction. A very large list can cause a slow transaction, excessive memory use from materializing all policy reads, or SQLite lock contention. Other RPC list inputs in this dispatcher (e.g., MEMORY_LIST_TRACES) cap via limit. Consider capping journalIds length here, consistent with how the preview's limit is capped to 500 in gain-maintenance.ts.

💡 Suggested Change

Before:

const journalIds = Array.isArray(p.journalIds)
          ? (p.journalIds as unknown[]).filter(
              (id): id is string => typeof id === "string" && id.length > 0,
            )
          : undefined;

After:

        const MAX_JOURNAL_IDS = 500;
        const journalIds = Array.isArray(p.journalIds)
          ? (p.journalIds as unknown[]).filter(
              (id): id is string => typeof id === 'string' && id.length > 0,
            ).slice(0, MAX_JOURNAL_IDS)
          : undefined;

4. apps/memos-local-plugin/bridge/methods.ts (L380)

Follow-up on the length cap above: if a cap is added, it should REJECT an over-long list rather than truncate it. Truncating would produce an unannounced partial rollback — the caller believes every supplied journal ID was reverted when only the first N were — which is precisely the failure mode that makes this destructive operation unsafe to use. Rejecting keeps the operation all-or-nothing, consistent with the batch-level ok: false conflict contract.

💡 Suggested Change

Before:

        const journalIds = Array.isArray(p.journalIds)

After:

        // Reject (do not truncate): a partially applied rollback must never be silent.
        if (journalIds !== undefined && journalIds.length > MAX_JOURNAL_IDS) {
          throw new MemosError(
            "invalid_argument",
            `${method}: too many 'journalIds' (max ${MAX_JOURNAL_IDS})`,
          );
        }
        const journalIds = Array.isArray(p.journalIds)

5. apps/memos-local-plugin/agent-contract/memory-core.ts (L467-L471)

The input type makes both batchId and journalIds optional with no compile-time mutual-exclusivity, so the TypeScript type accepts three invalid states at compile time: both absent, both present, and journalIds as an empty array. The implementation in gain-maintenance.ts throws at runtime for all three, but the contract type gives callers no static signal of that constraint.

The destructive scope of this operation (rewrites gain/gain_version/status for every matched journal row and parks queue entries blocked) means a misused call is difficult to recover from. Modelling the selector as a discriminated union would make invalid calls a compile error instead of a runtime rejection:

rollbackGainRepair(input:
  | { namespace: RuntimeNamespace; batchId: string; journalIds?: never }
  | { namespace: RuntimeNamespace; journalIds: readonly string[]; batchId?: never }
): Promise<GainRollbackResult>;

6. apps/memos-local-plugin/agent-contract/memory-core.ts (L444-L448)

The implementation in gain-maintenance.ts fetches up to 100,000 policies via repos.policies.list({ limit: 100_000 }), runs recomputePolicyGain for every one of them in JS, then discards all but a single page of results. Memory and CPU cost therefore scale with the full candidate/active universe of the namespace, not with the requested page size. For a namespace with tens of thousands of policies this makes every preview call expensive regardless of the limit/offset parameters the caller passes, which contradicts the expectation that pagination controls cost.

The SQL-side total count is already scoped correctly; the recompute loop should be similarly bounded to the requested page, or the preview should accept that it is an expensive full-scan and document the cost ceiling so callers are not surprised.


7. apps/memos-local-plugin/agent-contract/memory-core.ts (L175-L187)

newGainVersion is typed as the literal union 1 | 2 while oldGainVersion is the open type number, making the two sides of the same repair incomparable without a cast. More concretely, the closed domain 1 | 2 is inconsistent with the open inferenceVersion: number fields on both GainPreviewQueueState and GainPreviewResult.

The migration confirms that gain_version is a plain INTEGER column with default 1, and the repair engine always writes version 2. If a third certification level is ever introduced, every assignment of the new version to newGainVersion will fail to typecheck — or the domain will be widened ad hoc in one place but not the other. A single named type (e.g. type GainVersion = number) shared by oldGainVersion, newGainVersion and the journal's newGainVersion field would eliminate the asymmetry and provide one place to tighten or widen the domain.


8. apps/memos-local-plugin/agent-contract/memory-core.ts (L198-L200)

The same condition is expressed twice in GainPreviewPolicyEntry: skipReason can be 'unknown_owner' and unknownOwner can be true, and the two are never guaranteed to agree by the type. The implementation in gain-maintenance.ts derives skipReason as recomputed.skipReason ?? (recomputed.unknownOwner ? 'unknown_owner' : null), so they are consistent when set, but the type permits incoherent combinations — unknownOwner: true with skipReason: null, or skipReason: 'unknown_owner' with unknownOwner: false — without a compile error.

Since GainPreviewSkipReason already contains 'unknown_owner', the boolean is redundant. Removing unknownOwner and letting consumers check skipReason === 'unknown_owner' eliminates the redundancy and removes the possibility of the two fields disagreeing.


9. apps/memos-local-plugin/agent-contract/memory-core.ts (L236)

The field discriminant in GainRollbackConflict uses mixed naming conventions: 'status', 'support', and 'gain' match the camelCase policy fields used throughout this contract, while 'gain_version' and 'updated_at' are the raw SQLite column names from migration 019. The implementation in gain-maintenance.ts assigns these literals directly from the CAS array and callers must match against them, so a consumer that exhaustively switches on this field must remember which values are camelCase and which are snake_case.

Convert 'gain_version''gainVersion' and 'updated_at''updatedAt' to be consistent with the rest of the contract surface.


10. apps/memos-local-plugin/core/memory/l2/gain.ts (L199)

Magic number 1 is used as the fallback gain version without a named constant. Across the codebase this value carries a specific domain meaning ("uncertified / pre-v2 calculation"). Defining a named constant (e.g. GAIN_VERSION_UNCERTIFIED = 1) and using it here would make the intent self-documenting and prevent silent breakage if the sentinel value ever changes.

💡 Suggested Change

Before:

    gainVersion: args.gainVersion ?? 1,

After:

    gainVersion: args.gainVersion ?? GAIN_VERSION_UNCERTIFIED,

11. apps/memos-local-plugin/core/memory/l2/gain-maintenance.ts (L120-L127)

previewGainRepair hydrates up to 100,000 policy rows and calls recomputePolicyGain on every one before applying the page slice (entries.slice(offset, offset + limit)). Each recomputePolicyGain call fans out into multiple repo reads (tracePolicyLinks, episodes, traces). For a namespace with tens of thousands of policies this is an unbounded synchronous N+1 scan that runs entirely before the caller receives a single entry, directly contradicting the module-level guarantee that preview is a lightweight sanity check. Consider recomputing only the page-sized slice after a lightweight sort on stored fields, or add a dedicated summary query for non-paged metrics.


12. apps/memos-local-plugin/core/memory/l2/gain-maintenance.ts (L420-L434)

The five-field CAS array is written out twice in near-identical form: once here in the compare loop and again verbatim inside deps.db.tx for drift detection. If the guarded field set or the ?? 1 gain-version default ever changes in one copy but not the other, the pre-transaction and in-transaction checks will silently diverge, allowing a concurrent write to go undetected or a legitimate rollback to be blocked. Extract a single helper such as buildCasPairs(policy, recorded) and call it in both phases.


13. apps/memos-local-plugin/core/memory/l2/gain-maintenance.ts (L516-L524)

c.policy is the compare-phase snapshot captured before the transaction. The drift check just above re-read the row as current and proved equality for the five CAS fields, but updateStats still writes support from the stale snapshot. Any field added to updateStats in the future that is not part of the CAS would be written from stale data, re-introducing exactly the hazard the in-transaction re-read was added to prevent. Use current.support (the freshly re-read row inside the transaction) instead.

💡 Suggested Change

Before:

      deps.repos.policies.updateStats(c.policyId, {
        // Restore ONLY the repair-owned fields; support is preserved and
        // updated_at is fresh — historical timestamps are never restored.
        support: c.policy.support,
        gain: c.oldGain,
        gainVersion: c.oldGainVersion,
        status: c.oldStatus,
        updatedAt: now,
      });

After:

      deps.repos.policies.updateStats(c.policyId, {
        support: current.support,
        gain: c.oldGain,
        gainVersion: c.oldGainVersion,
        status: c.oldStatus,
        updatedAt: now,
      });

14. apps/memos-local-plugin/core/memory/l2/gain-maintenance.ts (L410-L419)

seenPolicies.set(policyKey, journalId) is called before the existence, ownership, and CAS checks. If the first journal row for policy P fails with policy_missing, not_found_or_forbidden, or policy_changed, a later journal row for the same P will be reported as duplicate_policy_entries instead of its real failure reason, misleading the operator. Move the seenPolicies.set call to just before candidates.push so only rows that passed all eligibility checks are recorded as first-seen.


15. apps/memos-local-plugin/core/memory/l2/gain-maintenance.ts (L116-L119)

total comes from count({...ownerFilter, statusIn: [...]}) but the returned entries come from a separate list(... limit: 100_000) that is then re-filtered in JS with isExactOwner. If the SQL owner pre-filter and the JS isExactOwner predicate ever disagree (e.g. a NULL workspace row matched by IS @workspace_id), or a namespace exceeds the 100,000 row cap, total will exceed the entries actually available: pagination then yields short or empty pages with no signal, and rows beyond the cap become silently invisible. Since this preview is the operator's pre-enablement assessment, under-reporting the universe can lead to a wrong go/no-go decision. Either derive total from the same filtered set, or drop the redundant JS filter so the two queries are guaranteed to agree.


16. apps/memos-local-plugin/core/memory/l2/gain-maintenance.ts (L100-L101)

Math.floor of a non-finite input yields NaN, and Math.max(1, Math.min(500, NaN)) propagates NaN. entries.slice(NaN, NaN) then silently returns an empty page instead of failing, which is inconsistent with the explicit invalid_argument errors used for the batchId/journalIds checks in the same file. Validate opts.limit/opts.offset are finite integers (or coerce with a fallback) before clamping, so a malformed request fails loudly rather than degrading into an empty result.


17. apps/memos-local-plugin/core/memory/l2/gain-repair.ts (L186-L192)

The { kind: "failed" } variant in GainRepairApplyOutcome is dead code. applyGainRepairAttempt never returns it — every code path returns "completed", "promoted", "blocked", "conflicted", or "reconciled". The matching case "failed" in runGainRepairTick's switch is therefore unreachable.

At the same time, the deleted-while-reserved path inside applyGainRepairAttempt writes result: "failed" to the journal but returns { kind: "reconciled" }, so the tick counts that wasted budget unit under counts.reconciled rather than counts.failed. Operators reading tick telemetry cannot distinguish a clean reconcile from a budget-consuming "policy vanished mid-attempt" event.

Suggestion: either remove { kind: "failed" } from the union and replace the deleted-policy path's return with { kind: "failed" } (so the journal result and the tick counter agree), or keep the reconcile return and drop the misleading failed journal write on that path.


18. apps/memos-local-plugin/core/memory/l2/gain-repair.ts (L618-L621)

The reconcile and re-screen phases are called without try/catch, while the per-policy reservation and apply phases are carefully wrapped. If reconcileInterruptedGainRepairClaims or consumeGainRepairRescreen throws (e.g. a SQLite locking error, a write constraint from markInterruptedJournal, or any exception inside the inference pass that consumeGainRepairRescreen runs), the exception propagates out of runGainRepairTick before the gain_repair.tick.done log and before any counts are returned. The timer's single-flight wrapper in memory-core.ts catches rejected promises, but the tick result is lost and the reconciled/rescreen state from earlier committed phases is invisible to callers.

Wrap both outer calls in try/catch with the same deps.log.warn pattern used for per-policy failures, and break/return a partial result rather than throwing from the tick orchestrator.


19. apps/memos-local-plugin/core/memory/l2/gain-repair.ts (L123-L130)

Two issues here:

  1. stored != null uses the loose inequality operator, which is prohibited by project style rules. Use stored !== null (the kv.get default is null, so undefined is not a concern here, but loose equality obscures that).

  2. The nested ternary maxTotal === null ? null : parsed === null ? 0 : Math.max(...) is also prohibited. Extract this into an explicit if/else or a small helper — the three distinct states (unlimited, corrupt/fail-closed, normal) deserve readable branching.

💡 Suggested Change

Before:

  return {
    attempted,
    limit: maxTotal,
    // Corrupt counters report zero remaining (fail closed) rather than a
    // fresh allowance.
    remaining: maxTotal === null ? null : parsed === null ? 0 : Math.max(0, maxTotal - attempted),
    initialized: stored != null,
  };

After:

  let remaining: number | null;
  if (maxTotal === null) {
    remaining = null;
  } else if (parsed === null) {
    // Corrupt counter: fail closed — report zero remaining so no new attempts are allowed.
    remaining = 0;
  } else {
    remaining = Math.max(0, maxTotal - attempted);
  }
  return {
    attempted,
    limit: maxTotal,
    remaining,
    initialized: stored !== null,
  };

20. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L359-L361)

In legacy mode (scoreMode === 'value', i.e. gainV2Enabled === false), isFirst reduces to policy.support === 0 only — the input.resetEma flag is completely ignored. recomputePolicyGain sets resetEma: input.resetEma === true || input.mode === 'inference_refresh' for every mode, so an inference_refresh recompute against a policy with support > 0 will land here with isFirst === false and smoothGain will EMA-blend the new raw gain with the old one. This directly contradicts the stated contract ('an inference-rule-refresh recompute also resets the EMA so a superseded inferred score never blends into its replacement'). The fix is to also check resetEma in the legacy branch:

const isFirst = isV2
  ? policy.support === 0 || (policy.gainVersion ?? 1) !== 2 || input.resetEma === true
  : policy.support === 0 || input.resetEma === true;

21. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L264)

A dedup helper is defined at the bottom of this file for exactly this operation, but it is not used here — Array.from(new Set(...)) is inlined instead. Use dedup(input.poolIds) to keep a single implementation.

💡 Suggested Change

Before:

  const allIds = Array.from(new Set(input.poolIds));

After:

  const allIds = dedup(Array.from(input.poolIds));

22. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L328-L358)

skipVersion and gainVersion are computed by the same scoreMode === 'gain' ? 2 : 1 expression in two separate declarations. Extract to a single constant at the top of the function body and reference it in both the skip path and the compute path to eliminate the duplication and the risk of them drifting.

💡 Suggested Change

Before:

  const skipVersion: 1 | 2 = input.scoreMode === "gain" ? 2 : 1;
  const skipResult = (skipReason: "no_resolved_with" | "unknown_owner") => ({
    skipReason,
    gainVersion: skipVersion,
    isFirst: false,
    raw: emptyGain(policy.id),
    persistedGain: policy.gain,
    ...base,
  });

  // Unknown-owner policies are excluded from automatic mutation:
  // the caller still receives the full selection report (preview relies on it)
  // but auto-mutation paths must not write gain/support/status for them.
  if (unknownOwner && input.rejectUnknownOwner === true) {
    return skipResult("unknown_owner");
  }

  if (selectedWith.length === 0) {
    return skipResult("no_resolved_with");
  }

  const raw = computeGain(
    {
      policyId: policy.id,
      withTraces: toTraceViews(selectedWith),
      withoutTraces: toTraceViews(selectedWithout),
    },
    { tauSoftmax: input.config.tauSoftmax },
  );
  const isV2 = input.scoreMode === "gain";
  const gainVersion: 1 | 2 = isV2 ? 2 : 1;

After:

  const gainVersion: 1 | 2 = input.scoreMode === "gain" ? 2 : 1;
  const skipResult = (skipReason: "no_resolved_with" | "unknown_owner") => ({
    skipReason,
    gainVersion,
    isFirst: false,
    raw: emptyGain(policy.id),
    persistedGain: policy.gain,
    ...base,
  });
  // ... (unchanged)
  const isV2 = gainVersion === 2;

23. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L639-L646)

Long line exceeds typical style limits and is difficult to scan. Split the compound condition onto separate lines.

💡 Suggested Change

Before:

export function isBorrowedEvidence(
  policyOwner: NormalizedOwner,
  trace: Pick<GainEvidenceTrace, "ownerAgentKind" | "ownerProfileId" | "ownerWorkspaceId">,
): boolean {
  const t = normalizeOwner(trace);
  if (t.kind === "unknown") return false;
  return !(t.kind === policyOwner.kind && t.profile === policyOwner.profile && t.workspace === policyOwner.workspace);
}

After:

export function isBorrowedEvidence(
  policyOwner: NormalizedOwner,
  trace: Pick<GainEvidenceTrace, "ownerAgentKind" | "ownerProfileId" | "ownerWorkspaceId">,
): boolean {
  const t = normalizeOwner(trace);
  if (t.kind === "unknown") return false;
  return !(
    t.kind === policyOwner.kind &&
    t.profile === policyOwner.profile &&
    t.workspace === policyOwner.workspace
  );
}

24. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L451)

The per-episode sort duplicates the byTsDescThenIdDesc comparator defined below, but operates on a different type (GainRow vs SelectedTrace) and uses localeCompare for the ID tie-break, as does the named comparator. Locale-sensitive collation is non-deterministic across Node builds and LANG settings. Because this sort controls which rows survive the NEWEST_PER_EPISODE cut, two identical datasets can produce different evidence pools on different machines. Use a locale-independent byte comparison for opaque IDs — String(b.id) < String(a.id) ? 1 : -1 — and ideally extract a shared comparator that both sites call so the documented selection contract has a single implementation.

💡 Suggested Change

Before:

    rows.sort((a, b) => b.ts - a.ts || String(b.id).localeCompare(String(a.id)));

After:

    rows.sort((a, b) => b.ts - a.ts || (String(b.id) < String(a.id) ? -1 : 1));

25. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L688-L690)

Same localeCompare non-determinism as the per-episode sort above. Trace IDs are opaque identifiers, not human text, so locale/ICU-dependent collation is unnecessary. Replace with a locale-independent byte comparison.

💡 Suggested Change

Before:

function byTsDescThenIdDesc(a: SelectedTrace, b: SelectedTrace): number {
  return b.row.ts - a.row.ts || b.id.localeCompare(a.id);
}

After:

function byTsDescThenIdDesc(a: SelectedTrace, b: SelectedTrace): number {
  return b.row.ts - a.row.ts || (b.id < a.id ? -1 : b.id > a.id ? 1 : 0);
}

26. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L436-L449)

The catch block is completely silent. A persistently failing traces.count call — for example a schema mismatch after a migration — would be invisible, while code proceeds as if the pool is complete. Log the error at debug level so it is diagnosable without affecting behavior.

💡 Suggested Change

Before:

    try {
      const total = deps.traces.count({ episodeId: epId as EpisodeId });
      const orphansExcluded = Math.max(0, total - rows.length);
      if (orphansExcluded > 0) {
        log.debug("recompute_gain.orphans_excluded", {
          policyId: String(policy.id),
          episodeId: epId,
          orphansExcluded,
          poolMembers: rows.length,
        });
      }
    } catch {
      // Diagnosability must never break selection — skip the signal.
    }

After:

    try {
      const total = deps.traces.count({ episodeId: epId as EpisodeId });
      const orphansExcluded = Math.max(0, total - rows.length);
      if (orphansExcluded > 0) {
        log.debug("recompute_gain.orphans_excluded", {
          policyId: String(policy.id),
          episodeId: epId,
          orphansExcluded,
          poolMembers: rows.length,
        });
      }
    } catch (err) {
      // Diagnosability must never break selection — skip the signal.
      log.debug("recompute_gain.orphan_count_failed", { episodeId: epId, err });
    }

27. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L618-L620)

isResolvedGainValue returns boolean rather than the type predicate gainValue is number. As a result, the call site in isInductionEligible must add a redundant gain != null check after calling it just to satisfy TypeScript narrowing — that check is logically unreachable because the helper already returns false for null/undefined. Returning a type predicate removes the dead condition and makes all call sites cleaner.

Also, the != null loose equality check (which covers both null and undefined) violates the project's strict-equality rule. Use gainValue !== null && gainValue !== undefined (or gainValue != null replaced with the predicate return) to be explicit about intent.

💡 Suggested Change

Before:

export function isResolvedGainValue(gainValue: number | null | undefined): boolean {
  return gainValue != null && Number.isFinite(gainValue) && Math.abs(gainValue) <= 1;
}

After:

export function isResolvedGainValue(gainValue: number | null | undefined): gainValue is number {
  return gainValue !== null && gainValue !== undefined && Number.isFinite(gainValue) && Math.abs(gainValue) <= 1;
}

28. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L595-L600)

ownerWorkspaceId is omitted from the seed watermark, while the queue rows themselves are keyed by (kind, profile, workspace). For a runtime that uses multiple distinct workspaces under the same kind/profile, the second workspace's reconcile call overwrites the marker written by the first. Any crash-recovery path that checks this marker to avoid double-seeding will incorrectly skip the remaining workspaces. Include ownerWorkspaceId in the stored object to make the marker namespace-exact.

💡 Suggested Change

Before:

    deps.kv.set(GAIN_REPAIR_QUEUE_SEED_KEY, {
      version,
      ownerAgentKind: owner.ownerAgentKind,
      ownerProfileId: owner.ownerProfileId,
      seededAt: now(),
    });

After:

    deps.kv.set(GAIN_REPAIR_QUEUE_SEED_KEY, {
      version,
      ownerAgentKind: owner.ownerAgentKind,
      ownerProfileId: owner.ownerProfileId,
      ownerWorkspaceId: owner.ownerWorkspaceId ?? null,
      seededAt: now(),
    });

29. apps/memos-local-plugin/core/memory/l2/recompute-gain.ts (L571-L578)

now() is evaluated once per policy inside the loop, so rows written in the same reconcile call carry slightly different timestamps. Since the reconcile is conceptually a single atomic operation, capture the timestamp once before the loop and reuse it.

💡 Suggested Change

Before:

      const common = {
        policyId: policy.id,
        ownerAgentKind: owner.ownerAgentKind,
        ownerProfileId: owner.ownerProfileId,
        ownerWorkspaceId: owner.ownerWorkspaceId ?? null,
        inferenceVersion: version,
        now: now(),
      };

After:

    const nowMs = now();
    for (const policy of targets) {
      // ...
      const common = {
        policyId: policy.id,
        ownerAgentKind: owner.ownerAgentKind,
        ownerProfileId: owner.ownerProfileId,
        ownerWorkspaceId: owner.ownerWorkspaceId ?? null,
        inferenceVersion: version,
        now: nowMs,
      };

30. apps/memos-local-plugin/core/pipeline/memory-core.ts (L822-L844)

l2ConfigSlice manually duplicates the exact same field mapping that extractAlgorithmConfig in deps.ts already produces. Every new L2Config field introduced in the future must be added to both places; the two sites are already slightly structured differently (e.g., inductionTraceCharCap vs traceCharCap aliasing). A safer approach is to re-use the existing builder:

function l2ConfigSlice(): L2Config {
  return extractAlgorithmConfig(handle.config.algorithm).l2Induction;
}

This eliminates the duplicate mapping and ensures the timer, preview, and rollback always see the exact same slice shape as the rest of the pipeline.


31. apps/memos-local-plugin/core/pipeline/memory-core.ts (L4837-L4839)

The upsertPending failure is silently swallowed with a bare catch {} and no logging. If the gainRepair repo is unavailable (e.g., migration 019 hasn't run, or the DB is in a degraded state), the error disappears entirely, making it impossible to diagnose why imported policies are absent from the repair queue. The comment's intent — not failing the import — is valid, but the error should still be surfaced at warn level so on-call operators can act:

} catch (err) {
  // Queue bookkeeping must never fail the policy import.
  log.warn("gain_repair.import_queue_failed", {
    policyId: dto.id,
    err: err instanceof Error ? err.message : String(err),
  });
}
💡 Suggested Change

Before:

            } catch {
              // Queue bookkeeping must never fail the policy import.
            }

After:

            } catch (err) {
              // Queue bookkeeping must never fail the policy import.
              log.warn("gain_repair.import_queue_failed", {
                policyId: dto.id,
                err: err instanceof Error ? err.message : String(err),
              });
            }

32. apps/memos-local-plugin/core/memory/l2/l2.ts (L126)

Association-only policies that are pre-loaded into touched (lines 348-354) but whose recomputePolicyGain returns a non-null skipReason hit continue in Step 4 and never reach the link flush block. Their intents stay in pendingLinks until the function returns and are silently dropped — no warning, no counter.

The previous code wrote links immediately, so this is a behavioral regression: an existing policy that was matched via cosine similarity in Step 1 will have no link row after the run, meaning recomputePolicyGain's getWithTraceIds call on the next run sees a smaller evidence set, under-counts support, and may keep the policy from advancing status.

The simplest fix is to flush any remaining intents in pendingLinks after the Step 4 loop, with a warning per dropped entry so the condition is observable in production logs.

💡 Suggested Change

Before:

      recordLinkIntent(a.matchedPolicyId, a.traceId as TraceId, input.episodeId);

After:

      // After Step 4 loop — flush any deferred links whose policy was skipped
      // (recomputed.skipReason !== null) so evidence is not silently lost.
      for (const [policyId, links] of pendingLinks) {
        for (const link of links) {
          try {
            repos.tracePolicyLinks.link({
              traceId: link.traceId,
              policyId,
              episodeId: link.episodeId,
              now: input.now ?? Date.now(),
            });
          } catch (err) {
            warnings.push(stageWarn("trace-policy-link", err, { traceId: link.traceId, policyId }));
          }
        }
      }
      pendingLinks.clear();

33. apps/memos-local-plugin/core/memory/l2/l2.ts (L282-L287)

In the merged-duplicate branch (and again in the newly-induced branch a few lines below), inductionEvidenceByPolicy.set overwrites any previously accumulated evidence for the same policy. If two ready buckets both resolve to the same duplicate policy, the second bucket's set call discards the first bucket's eligible IDs.

The candidate-pool duplicate branch (the dup path above) correctly uses union semantics (const evidence = inductionEvidenceByPolicy.get(dup.id) ?? new Set(); evidence.add(...)). The merged-duplicate and new-policy branches should do the same — merge into the existing set rather than replace it.

💡 Suggested Change

Before:

        inductionEvidenceByPolicy.set(duplicate.id, new Set(eligibleEvidenceIds));
        for (const traceId of eligibleEvidenceIds) {
          const trace = traces.find((t) => t.id === traceId);
          if (!trace) continue;
          recordLinkIntent(duplicate.id, traceId as TraceId, trace.episodeId);
        }

After:

        const existingEvidence = inductionEvidenceByPolicy.get(duplicate.id) ?? new Set<string>();
        for (const id of eligibleEvidenceIds) existingEvidence.add(id);
        inductionEvidenceByPolicy.set(duplicate.id, existingEvidence);
        for (const traceId of eligibleEvidenceIds) {
          const trace = traces.find((t) => t.id === traceId);
          if (!trace) continue;
          recordLinkIntent(duplicate.id, traceId as TraceId, trace.episodeId);
        }

34. apps/memos-local-plugin/core/memory/l2/l2.ts (L318-L323)

Same overwrite problem as in the merged-duplicate branch: inductionEvidenceByPolicy.set replaces any existing set for policy.id instead of unioning into it. A newly-inducted policy could theoretically be matched from two buckets in the same run (e.g., two signatures that both deduplicate to the same content hash), and the second write silently loses the first bucket's evidence.

💡 Suggested Change

Before:

        inductionEvidenceByPolicy.set(policy.id, new Set(eligibleEvidenceIds));
        for (const traceId of eligibleEvidenceIds) {
          const trace = traces.find((t) => t.id === traceId);
          if (!trace) continue;
          recordLinkIntent(policy.id, traceId as TraceId, trace.episodeId);
        }

After:

        const existingEvidence = inductionEvidenceByPolicy.get(policy.id) ?? new Set<string>();
        for (const id of eligibleEvidenceIds) existingEvidence.add(id);
        inductionEvidenceByPolicy.set(policy.id, existingEvidence);
        for (const traceId of eligibleEvidenceIds) {
          const trace = traces.find((t) => t.id === traceId);
          if (!trace) continue;
          recordLinkIntent(policy.id, traceId as TraceId, trace.episodeId);
        }

35. apps/memos-local-plugin/core/memory/l2/l2.ts (L579-L585)

The cast (policy.ownerAgentKind ?? "unknown") as RuntimeNamespace["agentKind"] fabricates a valid-looking namespace when ownerAgentKind is null or holds a value outside the RuntimeNamespace union. If recomputePolicyGain uses this namespace for ownership filtering, the policy will match zero owned traces, recomputed.skipReason will be non-null, and the policy will be silently skipped on every run with its pendingLinks entries dropped — a permanent no-op with no error surfaced.

Additionally, the inline import("../../types.js").RuntimeNamespace dynamic type reference is inconsistent with the file's existing top-level import type { ... } from "../../types.js" block and hurts readability. Move RuntimeNamespace into the top-level import and add a runtime guard instead of the bare cast.

💡 Suggested Change

Before:

function namespaceFromPolicy(policy: PolicyRow): import("../../types.js").RuntimeNamespace {
  return {
    agentKind: (policy.ownerAgentKind ?? "unknown") as import("../../types.js").RuntimeNamespace["agentKind"],
    profileId: policy.ownerProfileId ?? "default",
    ...(policy.ownerWorkspaceId ? { workspaceId: policy.ownerWorkspaceId } : {}),
  };
}

After:

// At the top-level import block, add RuntimeNamespace:
// import type { ..., RuntimeNamespace } from "../../types.js";

function namespaceFromPolicy(policy: PolicyRow): RuntimeNamespace {
  const agentKind = policy.ownerAgentKind;
  if (!agentKind || agentKind === "unknown") {
    // ownerAgentKind is unset; evidence owner-matching will use the
    // policy's stored fields directly via normalizeOwner inside recomputePolicyGain.
    // Return a minimal namespace — the helper ignores this field anyway.
    return { agentKind: "unknown" as RuntimeNamespace["agentKind"], profileId: policy.ownerProfileId ?? "default" };
  }
  return {
    agentKind: agentKind as RuntimeNamespace["agentKind"],
    profileId: policy.ownerProfileId ?? "default",
    ...(policy.ownerWorkspaceId ? { workspaceId: policy.ownerWorkspaceId } : {}),
  };
}

36. apps/memos-local-plugin/core/memory/l2/l2.ts (L491-L499)

A candidate blocked by a pending inference_refresh entry can stay blocked permanently if the refresh is never completed (LLM unavailable, claim stuck, repeated failure). The continue only logs at info level and emits no counter or warning, making permanent starvation indistinguishable from normal transient deferral in production.

Additionally, the unknown-owner continue directly above this block logs nothing at all, so the two skips are indistinguishable in log output.

Consider emitting a warn-level log (or incrementing a counter) when a policy has been blocked past a configurable age threshold, and adding at least a distinct log field for the unknown-owner skip.


37. apps/memos-local-plugin/core/memory/l2/l2.ts (L191-L197)

Evidence revalidation silently narrows epIds by deriving it from the surviving traces rather than bucket.episodeIds. A bucket that previously had enough episodes to qualify for induction can now fail minEpisodesForInduction purely because traces were filtered out by the gainValue floor or were missing from the repo — but the skippedReason remains "too_few_episodes" with no distinction between ineligible traces and missing ones.

This makes it hard to debug why induction regressed after a gainV2Enabled flip. At minimum, compute the counts before and after filtering and include them in the skip record or a log entry.

💡 Suggested Change

Before:

      const traces = bucket.evidenceTraceIds
        .map((id) => repos.traces.getById(id))
        .filter((t): t is TraceRow => !!t)
        .filter((t) => isInductionEligible(t, config));
      const eligibleEvidenceIds = traces.map((t) => t.id);
      const epIds = Array.from(new Set(traces.map((t) => t.episodeId))) as EpisodeId[];
      if (traces.length === 0 || epIds.length < config.minEpisodesForInduction) {

After:

      const allFetchedTraces = bucket.evidenceTraceIds
        .map((id) => repos.traces.getById(id))
        .filter((t): t is TraceRow => !!t);
      const traces = allFetchedTraces.filter((t) => isInductionEligible(t, config));
      const ineligibleCount = allFetchedTraces.length - traces.length;
      const missingCount = bucket.evidenceTraceIds.length - allFetchedTraces.length;
      const eligibleEvidenceIds = traces.map((t) => t.id);
      const epIds = Array.from(new Set(traces.map((t) => t.episodeId))) as EpisodeId[];
      if (traces.length === 0 || epIds.length < config.minEpisodesForInduction) {
        // include ineligibleCount / missingCount in the skip record for diagnosability
        inductions.push({
          signature: bucket.signature,
          policyId: null,
          poolSize: bucket.candidateIds.length,
          episodeIds: epIds,
          traceIds: eligibleEvidenceIds,
          skippedReason: "too_few_episodes",
          // diagnostics
          _ineligibleTraceCount: ineligibleCount,
          _missingTraceCount: missingCount,
        });

38. apps/memos-local-plugin/core/memory/l2/l2.ts (L227-L231)

traces.find((t) => t.id === traceId) is an O(n) scan inside a loop over eligibleEvidenceIds, giving O(n²) work per bucket. This same pattern appears in three separate branches (pool-duplicate, merged-duplicate, new-policy). Build a Map<string, TraceRow> once from traces before the branching logic and look up by key — and consider extracting the shared link-intent loop into a local helper to remove the triplication.

💡 Suggested Change

Before:

        for (const traceId of eligibleEvidenceIds) {
          const trace = traces.find((t) => t.id === traceId);
          if (!trace) continue;
          recordLinkIntent(dup.id, traceId as TraceId, trace.episodeId);
        }

After:

      // Build once per bucket; reused across all three branches below.
      const traceById = new Map(traces.map((t) => [t.id, t]));

      // Then in each branch replace the find with:
      for (const traceId of eligibleEvidenceIds) {
        const trace = traceById.get(traceId);
        if (!trace) continue;
        recordLinkIntent(dup.id, traceId as TraceId, trace.episodeId);
      }

39. apps/memos-local-plugin/core/memory/l2/l2.ts (L631-L640)

The score closure is re-created on every loop iteration even though its shape depends only on config.gainV2Enabled, which is constant for the lifetime of the call. Hoist it above the loop to allocate it once.

💡 Suggested Change

Before:

function pickOnePerEpisode(traces: readonly TraceRow[], config: L2Config): TraceRow[] {
  const byEp = new Map<string, TraceRow>();
  for (const t of traces) {
    const cur = byEp.get(t.episodeId);
    // representative selection uses the evidence score of the
    // enabled mode (gainValue when v2 is on; the legacy V otherwise).
    const score = (tr: TraceRow): number =>
      config.gainV2Enabled ? (tr.gainValue ?? tr.value) : tr.value;
    if (!cur || score(t) > score(cur)) byEp.set(t.episodeId, t);
  }

After:

function pickOnePerEpisode(traces: readonly TraceRow[], config: L2Config): TraceRow[] {
  const score = (tr: TraceRow): number =>
    config.gainV2Enabled ? (tr.gainValue ?? tr.value) : tr.value;
  const byEp = new Map<string, TraceRow>();
  for (const t of traces) {
    const cur = byEp.get(t.episodeId);
    if (!cur || score(t) > score(cur)) byEp.set(t.episodeId, t);
  }

40. apps/memos-local-plugin/core/reward/gain-inference.ts (L146-L150)

The ownership validation block is skipped entirely when episodeOwnerAgentKind and episodeOwnerProfileId are both empty strings (""), since empty strings are falsy in JavaScript. If the episode owner fields are stored as empty strings rather than null/"unknown"/"default", a member belonging to a different agent will pass the ownership check unchallenged.

The outer gate should use != null (or !== undefined && !== null) instead of truthiness, and the inner per-field comparisons should follow the same pattern so an empty-string owner is actually enforced.

💡 Suggested Change

Before:

  if (
    input.episodeOwnerAgentKind ||
    input.episodeOwnerProfileId ||
    input.episodeOwnerWorkspaceId !== undefined
  ) {

After:

  if (
    input.episodeOwnerAgentKind != null ||
    input.episodeOwnerProfileId != null ||
    input.episodeOwnerWorkspaceId !== undefined
  ) {
    for (const m of members) {
      const mKind = m.ownerAgentKind ?? "unknown";
      const mProfile = m.ownerProfileId ?? "default";
      const mWorkspace = m.ownerWorkspaceId ?? null;
      if (
        (input.episodeOwnerAgentKind != null && mKind !== input.episodeOwnerAgentKind) ||
        (input.episodeOwnerProfileId != null && mProfile !== input.episodeOwnerProfileId) ||
        (input.episodeOwnerWorkspaceId !== undefined && mWorkspace !== episodeWorkspace)
      ) {
        return unresolved("mixed_ownership");
      }
    }
  }

41. apps/memos-local-plugin/core/reward/gain-inference.ts (L524-L536)

When a trace ID in S is not found in byId (ghost/missing trace), the code silently substitutes sentinel values: value: Number.NaN, rHuman: null, ts: 0. The group correctly ends up unresolved because screenGainGroup rejects non-finite values and null rHuman, but the underlying data-integrity problem — a trace ID listed in the episode's reward-pass set that doesn't exist in the database — is never separately flagged or counted. It is silently absorbed into the unresolved bucket, making it indistinguishable from a legitimately unresolvable group.

Additionally, contributionGainValues (confirmed in gain-value.ts) throws a RangeError on non-finite inputs. Although the inferred_normalized path is only reached after screenGainGroup validates all values, a future code path change could expose this throw inside the deps.db.tx() call, rolling back the transaction silently.

Recommendation: detect ghost members before constructing the input to screenGainGroup, log/count them separately (e.g. a ghostMembers field in the report), and short-circuit to unresolved("ghost_member") with a distinct reason code so operators can distinguish missing traces from integrity failures.

💡 Suggested Change

Before:

        members: S.map((tid) => {
          const tr = byId.get(String(tid));
          return {
            id: String(tid),
            episodeId: tr ? String(tr.episodeId) : id,
            value: tr?.value ?? Number.NaN,
            rHuman: tr?.rHuman ?? null,
            ts: tr?.ts ?? 0,
            ownerAgentKind: tr?.ownerAgentKind ?? "unknown",
            ownerProfileId: tr?.ownerProfileId ?? "default",
            ownerWorkspaceId: tr?.ownerWorkspaceId ?? null,
          };
        }),

After:

      const ghostIds = S.filter((tid) => !byId.has(String(tid)));
      if (ghostIds.length > 0) {
        log.warn("gain_inference.ghost_members", { episodeId: id, ghostIds });
        // stamp existing real members unresolved and move on
        // ... (same stamp loop as the idList === null branch)
        report.unresolved.groups += 1;
        continue;
      }
      const outcome = screenGainGroup({
        episodeId: id,
        traceIds: S,
        members: S.map((tid) => {
          const tr = byId.get(String(tid))!; // safe: ghost check above
          return {
            id: String(tid),
            episodeId: String(tr.episodeId),
            value: tr.value,
            rHuman: tr.rHuman ?? null,
            ts: tr.ts ?? 0,
            ownerAgentKind: tr.ownerAgentKind ?? "unknown",
            ownerProfileId: tr.ownerProfileId ?? "default",
            ownerWorkspaceId: tr.ownerWorkspaceId ?? null,
          };
        }),
        // ...
      });

42. apps/memos-local-plugin/core/reward/gain-inference.ts (L579-L584)

Nested ternary expression. The review checklist explicitly prohibits nested ternaries. Replace with an if/else if/else block or a lookup map for clarity and future maintainability.

💡 Suggested Change

Before:

      const counts =
        outcome.status === "inferred_normalized"
          ? report.inferredNormalized
          : outcome.status === "legacy_unscaled"
            ? report.legacyUnscaled
            : report.unresolved;

After:

      let counts: GainInferenceCounts;
      if (outcome.status === "inferred_normalized") {
        counts = report.inferredNormalized;
      } else if (outcome.status === "legacy_unscaled") {
        counts = report.legacyUnscaled;
      } else {
        counts = report.unresolved;
      }

43. apps/memos-local-plugin/core/reward/reward.ts (L292-L296)

When deps.db is provided, all updateScore calls run in a single transaction. If any statement inside writeScores throws (e.g. a mid-batch SQL error), the entire transaction rolls back and zero traces get their V/alpha/priority updated — strictly worse than the pre-change behaviour where partial writes were possible. The gainOk guard only protects against a gain-computation failure that happens before entering the write loop; it does not protect against a SQL failure that occurs on the 50th trace of 100. The surrounding catch block then records only a warning and the run proceeds as if all scores were persisted, so the caller and all reward.updated subscribers have no way to know that no scores were actually written.

Consider either (a) catching SQL failures inside writeScores and falling back to writing V/alpha/priority without gain keys, or (b) re-throwing from the catch block when the persist step fails entirely so the caller observes the failure rather than a silent no-op.


44. apps/memos-local-plugin/core/reward/reward.ts (L277-L290)

The 'live_normalized' string is inlined here with an as const cast rather than referencing a shared constant. The traces repo already defines const LIVE_GAIN_SOURCE: GainValueSource = 'live_normalized' (used to build SQL guards) and the migration CHECK constraint enumerates the same literal. Having three independent string literals means a future rename or typo in one place silently breaks the runtime guard that prevents historical inference from overwriting live rows — because that guard is a SQL != 'live_normalized' comparison that would never match a mis-spelled source value written here.

Export and import LIVE_GAIN_SOURCE (or an equivalent shared constant from types.ts) so all producers and consumers share one type-checked literal.


45. apps/memos-local-plugin/core/reward/reward.ts (L257-L261)

The stage label "persist.traces.gain" is misleading: the failure being caught here is a computation failure (contributionGainValues threw), not a persistence failure. The persist step hasn't been reached yet. This makes triage harder — an operator seeing this warning in logs will look at the SQL/storage layer first. Use a label that reflects the actual failing stage, e.g. "score.gain_compute", consistent with the "score.*" naming pattern used elsewhere in this runner.


46. apps/memos-local-plugin/core/storage/migrations/019-policy-gain-value.sql (L39-L40)

Missing index on traces.gain_inference_version. The listTraceIdsStampedAt method in traces.ts queries WHERE gain_inference_version = @version AND gain_inference_version > 0 and the selectCandidates prepared statement in gain-inference.ts filters on t.gain_inference_version < @version. Both run over the full traces table with no covering index on this column. On a large dataset, the boot-time inference pass and the queue-seed reconciliation will each execute a full table scan, risking a timeout against the 120 s watchdog (GAIN_INFERENCE_BOOT_TIME_BUDGET_MS = 30_000). Consider adding:

CREATE INDEX IF NOT EXISTS idx_traces_gain_inference_version
  ON traces(owner_agent_kind, owner_profile_id, gain_inference_version)
  WHERE gain_inference_version = 0;

or at minimum a plain CREATE INDEX … ON traces(gain_inference_version).


47. apps/memos-local-plugin/core/storage/migrations/019-policy-gain-value.sql (L108-L109)

gain_repair_journal.new_updated_at is defined via a trailing ALTER TABLE after the CREATE TABLE STRICT statement. SQLite ALTER TABLE ADD COLUMN on a STRICT table cannot carry CHECK constraints, GENERATED expressions, or DEFAULT expressions referencing other columns. While this specific column is a plain nullable INTEGER (no constraint), splitting the column definition out of the CREATE body sets a risky precedent: any future maintainer adding a constraint to this ALTER will get a silent no-op or a runtime error, not a compile-time failure. More critically, this migration is not wrapped in a single transaction — if the CREATE TABLE succeeds but the process crashes before the ALTER TABLE, the table exists without new_updated_at, and any subsequent re-run will fail because IF NOT EXISTS makes the CREATE a no-op while the ALTER TABLE raises duplicate column name. Consider moving new_updated_at into the CREATE TABLE body directly:

  new_updated_at INTEGER
) STRICT;

48. apps/memos-local-plugin/core/storage/migrations/019-policy-gain-value.sql (L89)

provenance_json uses CHECK (json_valid(provenance_json)), but SQLite's json_valid() accepts any valid JSON — objects, numbers, booleans, null — not only arrays. If application code emits '{}' or 'null' instead of '[]' due to a serialization bug, the constraint passes silently. Downstream TypeScript in gain-inference.ts and the repair repo iterates provenance_json as an array; a non-array value would cause a runtime error or corrupt gain calculations. A tighter guard would be:

CHECK (json_valid(provenance_json) AND json_type(provenance_json) = 'array')

49. apps/memos-local-plugin/core/storage/repos/policies.ts (L142-L145)

Using IS instead of = for a parameterized comparison is non-standard SQL and its behavior is SQLite-specific. In SQLite, IS treats NULL as equal to NULL (i.e., NULL IS NULL is true), which is different from =. However, when filter.ownerWorkspaceId is a non-null string, col IS @param and col = @param behave identically — so the intent here is almost certainly to handle the case where the filter value is null (to find rows where owner_workspace_id IS NULL).

The problem is that this conflates two distinct operations in a single branch: when filter.ownerWorkspaceId is a non-null string it should use = @param, and when it is explicitly null it should emit owner_workspace_id IS NULL with no parameter. Relying on IS @param to silently handle both cases is fragile and will break if this code ever runs on a non-SQLite database. Split the two cases explicitly:

if (filter.ownerWorkspaceId !== undefined) {
  if (filter.ownerWorkspaceId === null) {
    fragments.push(`owner_workspace_id IS NULL`);
  } else {
    fragments.push(`owner_workspace_id = @owner_workspace_id`);
    params.owner_workspace_id = filter.ownerWorkspaceId;
  }
}

This same pattern appears in both list() and count() and should be fixed in both places.

💡 Suggested Change

Before:

      if (filter.ownerWorkspaceId !== undefined) {
        fragments.push(`owner_workspace_id IS @owner_workspace_id`);
        params.owner_workspace_id = filter.ownerWorkspaceId ?? null;
      }

After:

      if (filter.ownerWorkspaceId !== undefined) {
        if (filter.ownerWorkspaceId === null) {
          fragments.push(`owner_workspace_id IS NULL`);
        } else {
          fragments.push(`owner_workspace_id = @owner_workspace_id`);
          params.owner_workspace_id = filter.ownerWorkspaceId;
        }
      }

50. apps/memos-local-plugin/core/storage/repos/policies.ts (L120-L126)

The statusIn filter-building block is duplicated verbatim in both list() and count(). Since both methods share the same PolicyListFilter type and identical fragment/param construction logic, this is a good candidate for a small private helper, e.g. applyStatusFilter(filter, fragments, params). Keeping the logic in one place avoids the risk of the two copies drifting apart in future changes.


51. apps/memos-local-plugin/core/storage/types.ts (L100-L101)

statusIn and status are silently mutually exclusive (the repo uses else if), but the interface exposes both fields with no indication that statusIn takes precedence and status is ignored when statusIn is non-empty. A caller who supplies both will be surprised.

Consider either:

  • Documenting the precedence rule inline, or
  • Choosing a single field (union type or overloaded filter) to eliminate the ambiguity entirely.
💡 Suggested Change

Before:

  /** Alternative to `status`: match any of the listed statuses in one query. */
  statusIn?: Array<"candidate" | "active" | "archived">;

After:

  /**
   * Alternative to `status`: match any of the listed statuses in one query.
   * When `statusIn` is provided and non-empty, `status` is ignored.
   */
  statusIn?: Array<"candidate" | "active" | "archived">;

52. apps/memos-local-plugin/core/storage/types.ts (L106)

ownerWorkspaceId?: string | null carries two distinct semantics: undefined = no filter applied, null = filter for rows where the column IS NULL. This distinction is critical for correctness but is invisible to callers from the type definition alone. Without an inline doc comment (unlike statusIn which has one), consumers are likely to conflate undefined and null and unintentionally match NULL-workspace rows—or fail to match them.

Add a JSDoc comment explaining the undefined vs null semantics.

💡 Suggested Change

Before:

  ownerWorkspaceId?: string | null;

After:

  /**
   * Filter by workspace owner.
   * - `undefined` (field omitted): no workspace filter applied.
   * - `null`: match only rows where `owner_workspace_id IS NULL`.
   * - `string`: match rows where `owner_workspace_id = <value>`.
   */
  ownerWorkspaceId?: string | null;

53. apps/memos-local-plugin/core/types.ts (L174)

The type is ?: number (optional, no null), but the comment uses 0 as the sentinel for "never screened". When a row pre-dates this column, the TypeScript layer will see undefined — creating two representations of the same state (undefined and 0). The sibling fields gainValue and gainValueSource are both ?: T | null, which lets a missing DB value surface as null and be distinguished from a deliberate zero. gainInferenceVersion should follow the same pattern so callers have a single unambiguous signal for "not yet written". Consider typing it as ?: number | null and treating null as "unresolved / never stamped" while 0 remains a valid stamped-but-unscreened value, or alternatively document that undefined and 0 are intentionally equivalent and use ?? 0 consistently at every read site.

💡 Suggested Change

Before:

  gainInferenceVersion?: number;

After:

  gainInferenceVersion?: number | null;

54. apps/memos-local-plugin/core/types.ts (L194)

Same inconsistency as gainInferenceVersion: the field is ?: number with no null, but a row that was never written will arrive as undefined while the migration default is 1. Callers that check gainVersion === 1 for "uncertified" will silently miss rows where the column was never set (value is undefined). Add | null to align with the other nullable policy/trace fields and make the "never written" state explicit, or document that every insert/upsert unconditionally sets this field so undefined is structurally impossible.

💡 Suggested Change

Before:

  gainVersion?: number;

After:

  gainVersion?: number | null;

55. apps/memos-local-plugin/core/storage/repos/gain-repair.ts (L447-L469)

GainRepairQueueState has exactly three variants (pending, blocked, claimed), so the maximum cardinality of states is 3. Building a dynamic IN (...) placeholder list and calling db.prepare() at runtime is unnecessary here and inconsistent with every other query in this file, all of which use pre-compiled statements. Even though StorageDb.prepare caches by SQL text, the generated SQL strings differ by placeholder count (@state_0, @state_0,@state_1, etc.), so callers with different subset sizes each produce a distinct cache entry. Replace with a pre-compiled statement that covers the full three-value IN list, or use three pre-compiled single-state statements and union the results, matching the module's established pattern.

💡 Suggested Change

Before:

    listByOwnerAndStates(
      owner: { ownerAgentKind: string; ownerProfileId: string; ownerWorkspaceId?: string | null },
      states: readonly GainRepairQueueState[],
    ): GainRepairQueueRow[] {
      if (states.length === 0) return [];
      const placeholders = states.map((_, i) => `@state_${i}`).join(",");
      const rows = db
        .prepare<{ kind: string; profile: string; workspace_id: string | null; [k: string]: unknown }, RawQueueRow>(
          `SELECT ${QUEUE_COLUMNS.join(", ")} FROM gain_repair_queue
           WHERE owner_agent_kind=@kind
             AND owner_profile_id=@profile
             AND owner_workspace_id IS @workspace_id
             AND state IN (${placeholders})
           ORDER BY policy_id`,
        )
        .all({
          kind: owner.ownerAgentKind,
          profile: owner.ownerProfileId,
          workspace_id: owner.ownerWorkspaceId ?? null,
          ...Object.fromEntries(states.map((s, i) => [`state_${i}`, s])),
        });
      return rows.map(mapQueueRow);
    },

After:

  // Pre-compiled statements for the fixed state subsets actually used by callers.
  // GainRepairQueueState has exactly 3 variants; dynamic SQL is unnecessary.
  const selectByOwnerAndStates = db.prepare<
    { kind: string; profile: string; workspace_id: string | null; s0: string; s1: string; s2: string },
    RawQueueRow
  >(
    `SELECT ${QUEUE_COLUMNS.join(", ")} FROM gain_repair_queue
     WHERE owner_agent_kind=@kind
       AND owner_profile_id=@profile
       AND owner_workspace_id IS @workspace_id
       AND state IN (@s0, @s1, @s2)
     ORDER BY policy_id`,
  );

  // ... inside the returned object:
  listByOwnerAndStates(
    owner: { ownerAgentKind: string; ownerProfileId: string; ownerWorkspaceId?: string | null },
    states: readonly GainRepairQueueState[],
  ): GainRepairQueueRow[] {
    if (states.length === 0) return [];
    // Pad to exactly 3 slots; SQLite ignores duplicates inside IN.
    const [s0, s1, s2] = [
      states[0],
      states[1] ?? states[0],
      states[2] ?? states[0],
    ];
    return selectByOwnerAndStates
      .all({ kind: owner.ownerAgentKind, profile: owner.ownerProfileId, workspace_id: owner.ownerWorkspaceId ?? null, s0, s1, s2 })
      .map(mapQueueRow);
  },

56. apps/memos-local-plugin/core/storage/repos/gain-repair.ts (L589-L603)

db.prepare(sql) is called inside the chunk loop, and the generated SQL string changes shape on every iteration because the IN (${placeholders}) list length varies with the actual chunk size (last chunk may be shorter than 900). Each distinct SQL string is a separate cache entry, so this loop incurs a fresh statement compilation for every chunk whose size differs from a prior one. For large affectedTraceIds sets this is a repeated compilation cost with no reuse. Consider extracting the per-chunk prepare outside the loop when the chunk size is fixed (all non-final chunks are exactly CHUNK_SIZE), or accepting that the final partial chunk compiles once and is thereafter cached.


57. apps/memos-local-plugin/core/storage/repos/gain-repair.ts (L734)

fromJsonText<string[]> is a generic wrapper around JSON.parse(raw) as T with no runtime type guard (confirmed in _helpers.ts). The fallback [] is only applied when parsing throws — not when parsing succeeds but returns a non-array (e.g. a JSON object {}, null, or a number 42). The provenance_json column has a json_valid() CHECK constraint but no constraint that enforces it is an array. Any caller iterating provenance as string[] would silently operate on the unexpected type. Add an Array.isArray guard after parsing, or apply it in the call site.

💡 Suggested Change

Before:

  provenance: fromJsonText<string[]>(r.provenance_json, []),

After:

  provenance: Array.isArray(fromJsonText<unknown>(r.provenance_json, []))
    ? (fromJsonText<string[]>(r.provenance_json, []))
    : [],
  // Or more efficiently, inline the guard:
  // provenance: (() => { const v = fromJsonText<unknown>(r.provenance_json, []); return Array.isArray(v) ? (v as string[]) : []; })(),

58. apps/memos-local-plugin/core/storage/repos/gain-repair.ts (L377-L380)

The selectJournalById query fetches any row matching id with no owner filter. The doc-comment delegates namespace authorization entirely to callers, but there is nothing in the query itself to prevent a caller from returning a row owned by a different namespace if it forgets the check. Since the journal contains per-policy gain deltas that can reveal cross-namespace policy existence and values, this is a meaningful information-disclosure risk. Adding AND owner_agent_kind=@kind AND owner_profile_id=@profile AND owner_workspace_id IS @workspace_id to the prepared statement enforces the contract at the storage layer rather than relying solely on every caller remembering to do it.

💡 Suggested Change

Before:

    getJournalById(id: string): GainRepairJournalRow | null {
      const r = selectJournalById.get({ id });
      return r ? mapJournalRow(r) : null;
    },

After:

  const selectJournalById = db.prepare<
    { id: string; kind: string; profile: string; workspace_id: string | null },
    RawJournalRow
  >(
    `SELECT ${JOURNAL_COLUMNS.join(", ")} FROM gain_repair_journal
      WHERE id=@id
        AND owner_agent_kind=@kind
        AND owner_profile_id=@profile
        AND owner_workspace_id IS @workspace_id`,
  );

  // update getJournalById signature accordingly:
  getJournalById(
    id: string,
    owner: { ownerAgentKind: string; ownerProfileId: string; ownerWorkspaceId?: string | null },
  ): GainRepairJournalRow | null {
    const r = selectJournalById.get({
      id,
      kind: owner.ownerAgentKind,
      profile: owner.ownerProfileId,
      workspace_id: owner.ownerWorkspaceId ?? null,
    });
    return r ? mapJournalRow(r) : null;
  },

59. apps/memos-local-plugin/core/storage/repos/traces.ts (L284-L297)

stampGain, unstampGainForRescreen, and listTraceIdsStampedAt all call db.prepare() inline inside the returned method body on every invocation. These three statements have fixed SQL with no variable structure, so there is no reason to compile them per-call. In a hot inference loop stamping thousands of traces, each call pays the full parse-and-plan cost and leaks a statement handle that is never explicitly finalized.

Move all three to the pre-compiled block at the top of makeTracesRepo alongside updateScoreBase, updateScoreWithGain, etc.:

const stampGainStmt = db.prepare<{ id: string; gain_value: number | null; gain_value_source: string | null; version: number }>(
  `UPDATE traces SET gain_value=@gain_value, gain_value_source=@gain_value_source,
   gain_inference_version=@version WHERE id=@id
   AND (gain_value_source IS NULL OR gain_value_source != ${SQL_LIVE_GAIN_SOURCE})`,
);
const unstampGainStmt = db.prepare<{ id: string }>(
  `UPDATE traces SET gain_value=NULL, gain_value_source=NULL, gain_inference_version=0
   WHERE id=@id AND (gain_value_source IS NULL OR gain_value_source != ${SQL_LIVE_GAIN_SOURCE})`,
);
const listStampedAtStmt = db.prepare<{ version: number }, { id: string }>(
  `SELECT id FROM traces WHERE gain_inference_version=@version AND gain_inference_version > 0`,
);

Then call .run()/.all() on those pre-compiled handles from each method.


60. apps/memos-local-plugin/core/storage/repos/traces.ts (L322-L324)

Same inline db.prepare() issue as in stampGain — fixed SQL compiled on every call. See the comment on stampGain for the fix.


61. apps/memos-local-plugin/core/storage/repos/traces.ts (L394-L399)

Same inline db.prepare() issue — fixed SQL, compiled on every call. Additionally the compound condition gain_inference_version = @version AND gain_inference_version > 0 is redundant: for any version > 0 the second predicate adds nothing, and for version = 0 the result is silently empty rather than throwing. If version=0 is an invalid input, enforce it explicitly; if it is valid, drop the > 0 guard. Either way, pre-compile the statement at init.


62. apps/memos-local-plugin/core/storage/repos/traces.ts (L208-L209)

updateScoreWithGain does not reset gain_inference_version to 0. If a trace was previously stamped by the historical inference pass (non-zero gain_inference_version) and is then updated via a live scoring call that goes through this statement, the column retains its old non-zero version while gain_value_source becomes live_normalized. listTraceIdsStampedAt(version) will then return that trace as if it were historically stamped at version, corrupting the queue-reconciliation seed set.

Add gain_inference_version=0 to the SET clause of updateScoreWithGain:

UPDATE traces
  SET value=@value, alpha=@alpha, r_human=@r_human, priority=@priority,
      gain_value=@gain_value, gain_value_source=@gain_value_source,
      gain_inference_version=0
WHERE id=@id

63. apps/memos-local-plugin/core/storage/repos/traces.ts (L382-L384)

Keyset pagination using id > @after_id with ORDER BY id is only correct if trace IDs are lexicographically monotone (e.g., ULIDs or time-prefixed). The TraceRow type declares id as a plain string, and if IDs are UUIDs (random) the ORDER BY id ordering is unrelated to insertion order. A caller iterating pages with afterId set to the last row's ID of the previous page will silently skip or duplicate rows, leaving some episode members permanently unstamped.

If IDs are not guaranteed to be monotone, change the pagination to use ts (with id as a tiebreaker for equal timestamps) and add the corresponding composite index, or document the ID format requirement explicitly and enforce it at generation time.


64. apps/memos-local-plugin/core/storage/repos/traces.ts (L375)

listGainRowsForEpisode silently caps results at 5000 with no signal to the caller that truncation occurred. The JSDoc says this is used to stamp malformed-episode members unresolved so they are never re-scanned — but if an episode has more than 5000 traces, the excess members are simply not returned, will not be stamped, and will be re-scanned on every restart.

Return a truncation indicator or throw for oversized episodes:

// Option A: expose a flag
return { rows: db.prepare(...).all(params).map(mapGainRow), truncated: rows.length === limit };

// Option B: assert the limit is never hit
if (rows.length === limit) {
  throw new Error(`listGainRowsForEpisode: episode ${episodeId} exceeds the ${limit}-row safety cap`);
}

🧹 Filtered 1 low-confidence OCR finding(s) before posting/fix-loop (existing_code_mismatch: 1).

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: ENV ISSUE

The test environment encountered an issue that requires manual attention.

Details: Environment preparation failed before any gating tests executed. Failed scopes: memos_local_plugin
Branch: pr/wp272-policy-gain-repair

L2 gain was a bare running average with no contributor-count scaling, so a
policy's gain floor-locks below the promotion threshold once enough
negative-evidence traces accumulate: recent strongly-positive evidence can
never lift it back over the line, the candidate is never promoted, and the
candidate pool grows without bound.

Score gain as clamp(N·V, -1, 1) — N the number of contributing episodes, V
the normalized reward credit — and persist it alongside V:

- Persist a contribution-adjusted `gainValue` per trace with versioned
  provenance (`live_normalized` / `inferred_normalized` / `legacy_unscaled`
  / unresolved), so partially-converted state is never observable.
- Add an idempotent, bounded historical-inference pass that runs once per
  boot, before the repair timer and its consumers start. Both bounds are
  config-tunable for hosts with a large backlog.
- Add a budgeted, timer-driven repair engine that drains the candidate and
  active backlog on a configurable cadence, instead of requiring a bulk
  one-shot migration.
- Add a read-only preview RPC (`policies.gainPreview`) and a policy-field
  CAS rollback RPC (`policies.gainRollback`) for safe operability.
- Migration 019 is schema-only: new columns plus the repair queue and
  journal tables. No data conversion, queue seeding, or policy/trace writes
  at migration time — all of that happens in the idempotent passes above.

Everything is gated behind `algorithm.l2Induction.gainV2Enabled` (default
false): an upgraded config is a no-op until an operator turns it on.
A large corpus can fail to converge its historical gain-value inference in
a single bridge restart under the built-in 30s / 2000-group bound. Expose
`gainInferenceBootMaxGroups` and `gainInferenceBootTimeBudgetMs` under
`algorithm.l2Induction` so a host can raise the budget for one restart
instead of requiring many manual restarts to converge.
@chiefmojo
chiefmojo force-pushed the pr/wp272-policy-gain-repair branch from b657e28 to d6a0a5a Compare September 16, 2026 03:05
`applyMigration` wraps migrations that touch `traces` in a `tableExists` check
(010/012/018): some harnesses build partial schemas where those tables are
absent, and migration 018's comment states the rule - the statement is
meaningless there and must not fail boot.

Migration 019 issued bare `ALTER TABLE traces` / `ALTER TABLE policies`, so it
aborted the migration pass on any partial schema. That is the regression behind
tests/unit/storage/migrator.test.ts ("treats embedding retry lease migration as
satisfied when columns already exist"), which passes on main and failed on this
branch.
@chiefmojo

chiefmojo commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Open Code Review findings — dispositions

Fixed

Rebutted

Deferred

None.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: ENV ISSUE

The test environment encountered an issue that requires manual attention.

Details: Environment preparation failed before any gating tests executed. Failed scopes: memos_local_plugin
Branch: pr/wp272-policy-gain-repair

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

Labels

area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants