Skip to content

Commit 1d7dfe5

Browse files
committed
fix(pull): warn loudly when disk-form hash fallback fires; docs/cosmetic polish
Address B1 (single action item) from the 3-commit follow-up code review: When `hashLocalResource` returns null after a successful writeResourceFile (e.g. broken file extension, race with rm, parseResourceDataFromFile throw), the silent fallback to `hashPayload(withCredNames)` reintroduces the exact M1 in-memory-vs-disk asymmetry that commit f296173 fixed \u2014 with NO log line for the next operator to find. They'd see phantom drift on the next pull and have no breadcrumb explaining why. Fix: capture diskHash separately, emit a `\u26a0\ufe0f` console.warn pointing at the specific resource when the fallback fires, then use the same nullish-coalesce. Bootstrap mode (no file written) is correctly excluded from the warning since the fallback there is expected. Also bundling 2 cosmetic nits from the same review: - A1: canonicalizeForHash JSDoc said pipeline mismatches make `lastPulledHash` disagree with the next read's `localHash`. The audit never writes lastPulledHash \u2014 it reads it. Correct framing is platformHash recomputation disagreeing with the stored lastPulledHash from a prior pull. - C2: summarizeFindings rendered '1 info' next to '1 error(s)' / '1 warning(s)' \u2014 inconsistent pluralization. Updated to '1 info finding(s)' for symmetry. Adjacent test assertion updated. Defer to follow-ups (non-blocking): - B2 bootstrap + manually-placed files edge case (narrow audience) - B3 perf measurement on 1000+-resource customers - C1 --no-info flag (monitor; add if customer complaints surface) - D1 split end-of-pull legend into 'skip reasons' + 'write outcomes' Tests: 266/266 green; tsc --noEmit clean.
1 parent 383018c commit 1d7dfe5

3 files changed

Lines changed: 24 additions & 11 deletions

File tree

src/audit.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -565,6 +565,6 @@ export function summarizeFindings(findings: AuditFinding[]): string {
565565
const warns = findings.filter((f) => f.severity === "warn").length;
566566
const infos = findings.filter((f) => f.severity === "info").length;
567567
const parts = [`${errors} error(s)`, `${warns} warning(s)`];
568-
if (infos > 0) parts.push(`${infos} info`);
568+
if (infos > 0) parts.push(`${infos} info finding(s)`);
569569
return `📋 Audit: ${findings.length} finding(s) — ${parts.join(", ")}`;
570570
}

src/pull.ts

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -516,9 +516,9 @@ export function resolveReferencesToResourceIds(
516516
* ALL hash sites MUST use this helper. The classifier (pull.ts), the audit
517517
* (audit.ts/checkContentDrift), and the pull-write fallback all need the
518518
* same transformation applied in the same order — any divergence (a missing
519-
* step, a different mutation order) makes the resulting `lastPulledHash`
520-
* disagree with the next read's `localHash`, producing permanent phantom
521-
* `both-diverged` reports that only `--overwrite` can clear.
519+
* step, a different mutation order) makes the recomputed `platformHash`
520+
* disagree with the stored `lastPulledHash` from a prior pull, producing
521+
* permanent phantom `both-diverged` reports that only `--overwrite` can clear.
522522
*
523523
* Note: this is the *pre-write* canonical form (in-memory). At write sites,
524524
* `lastPulledHash` should still be sourced from `hashLocalResource(...)` after
@@ -1018,11 +1018,20 @@ export async function pullResourceType(
10181018
// code review on the drift-direction-classifier PR.
10191019
//
10201020
// Bootstrap mode writes no file; fall back to the in-memory canonical form.
1021+
// Warn loudly when the disk-form hash fails on a written file — silent
1022+
// fallback would reintroduce the M1 asymmetry with no diagnostic for the
1023+
// next operator to find.
1024+
const diskHash = bootstrap
1025+
? null
1026+
: hashLocalResource(resourceType, resourceId);
1027+
if (!bootstrap && diskHash === null) {
1028+
console.warn(
1029+
` ⚠️ ${resourceType}/${resourceId}: failed to hash post-write disk form; falling back to in-memory hash (may produce phantom drift on next pull)`,
1030+
);
1031+
}
10211032
upsertState(newStateSection, resourceId, {
10221033
uuid: resource.id,
1023-
lastPulledHash:
1024-
hashLocalResource(resourceType, resourceId) ??
1025-
hashPayload(withCredNames),
1034+
lastPulledHash: diskHash ?? hashPayload(withCredNames),
10261035
lastPulledAt: new Date().toISOString(),
10271036
});
10281037
}
@@ -1110,11 +1119,15 @@ async function resolveBothDivergedResources(options: {
11101119
` ⬇️ ${entry.resourceId} (both diverged — resolving with --resolve=theirs, overwriting local with platform) ${formatDriftLabel("both-diverged")}`,
11111120
);
11121121
// Hash the post-write disk form (same invariant as the normal pull-write path).
1122+
const diskHash = hashLocalResource(entry.resourceType, entry.resourceId);
1123+
if (diskHash === null) {
1124+
console.warn(
1125+
` ⚠️ ${entry.resourceType}/${entry.resourceId}: failed to hash post-write disk form; falling back to in-memory hash (may produce phantom drift on next pull)`,
1126+
);
1127+
}
11131128
upsertState(section, entry.resourceId, {
11141129
uuid: entry.resource.id,
1115-
lastPulledHash:
1116-
hashLocalResource(entry.resourceType, entry.resourceId) ??
1117-
hashPayload(withCredNames),
1130+
lastPulledHash: diskHash ?? hashPayload(withCredNames),
11181131
lastPulledAt: new Date().toISOString(),
11191132
});
11201133
}

tests/audit.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -588,7 +588,7 @@ test("summarizeFindings: info-severity count appears in the summary line when pr
588588
];
589589
const out = summarizeFindings(findings);
590590
assert.match(out, /1 warning/);
591-
assert.match(out, /1 info/);
591+
assert.match(out, /1 info finding/);
592592
assert.match(out, /2 finding/);
593593
});
594594

0 commit comments

Comments
 (0)