[Review] computeVaultConflictSummary always returns avgRepairDays = 0 — silent lie when no schema support exists #186

New issue

Closed

opened 2026-09-14 14:07:30 +00:00 by Mark · 0 comments

Mark commented

2026-09-14 14:07:30 +00:00

Owner

Finding

src/vault/vault-metrics-engine.ts:178-188:

// Resolve-time estimation only meaningful when a conflict entry is paired
// with its resolution — without resolution timing data on each entry we
// fall back to null. The schema deliberately leaves this extensible.
for (const conflict of sorted) {
  if (conflict.resolved && conflict.interactionId != null) {
    repairDurations.push(0);
  }
}
if (repairDurations.length > 0) {
  avgRepairDays = Math.round(
    repairDurations.reduce((acc, v) => acc + v, 0) / repairDurations.length,
  );
}

The function pushes a literal 0 for every resolved conflict's "repair duration." Then takes the average. So avgRepairDays always reports 0 days. The comment says "without resolution timing data on each entry we fall back to null" but the code falls back to 0, not null.

The return type allows number | null. The user sees "Average repair time: 0 days" which is either always-zero or always-a-lie.

Suggested fix

Return null when no real repair-duration data is available:

let avgRepairDays: number | null = null;
// ...
if (repairDurations.length > 0) {
  avgRepairDays = Math.round(...);
}

The repairDurations.push(0) line should be removed entirely — pushing a constant provides no information. When the schema gains resolvedAt on VaultConflictEntry later, the real computation can be added back.

Severity

Low. Cosmetic / honesty fix. No data is wrong; the displayed value is misleading.

Discovered by

Manual code review pass over #169, 2026-09-14.

## Finding `src/vault/vault-metrics-engine.ts:178-188`: ```ts // Resolve-time estimation only meaningful when a conflict entry is paired // with its resolution — without resolution timing data on each entry we // fall back to null. The schema deliberately leaves this extensible. for (const conflict of sorted) { if (conflict.resolved && conflict.interactionId != null) { repairDurations.push(0); } } if (repairDurations.length > 0) { avgRepairDays = Math.round( repairDurations.reduce((acc, v) => acc + v, 0) / repairDurations.length, ); } ``` The function pushes a literal `0` for every resolved conflict's "repair duration." Then takes the average. So `avgRepairDays` always reports `0` days. The comment says "without resolution timing data on each entry we fall back to null" but the code falls back to `0`, not `null`. The return type allows `number | null`. The user sees "Average repair time: 0 days" which is either always-zero or always-a-lie. ## Suggested fix Return `null` when no real repair-duration data is available: ```ts let avgRepairDays: number | null = null; // ... if (repairDurations.length > 0) { avgRepairDays = Math.round(...); } ``` The `repairDurations.push(0)` line should be removed entirely — pushing a constant provides no information. When the schema gains `resolvedAt` on `VaultConflictEntry` later, the real computation can be added back. ## Severity Low. Cosmetic / honesty fix. No data is wrong; the displayed value is misleading. ## Discovered by Manual code review pass over #169, 2026-09-14.