Skip to content

Commit ee855dc

Browse files
committed
fix(notion-fetch): correct skip reason logic to detect impossible conditions
Fix logic bug in skip reason detection where unreachable branches were incorrectly labeled as "should not skip" instead of being identified as logic errors. Problem: - Inside `if (!needsProcessing)` block, we know ALL conditions for needsProcessing are FALSE due to boolean OR logic - Previous labels like "should not skip" were misleading since those branches are mathematically unreachable - Would never detect actual logic bugs in needsProcessing calculation Solution: - Change impossible condition messages to "🔴 ERROR:" prefix - Add clear comments explaining unreachability - Document that ONLY "unchanged since [timestamp]" is valid - If ERROR appears in logs, indicates bug in needsProcessing logic Benefits: - Proper detection of logic bugs (if they exist) - Clear distinction between valid skip (cache hit) vs impossible cases - Better debugging when investigating Issue #95 Related: ISSUE_95_PLAN.md Phase 1 review
1 parent 3859501 commit ee855dc

File tree

1 file changed

+19
-7
lines changed

1 file changed

+19
-7
lines changed

scripts/notion-fetch/generateBlocks.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -712,23 +712,35 @@ export async function generateBlocks(
712712

713713
if (!needsProcessing) {
714714
// Page unchanged, skip processing but still count it
715-
// Determine the specific reason for skipping (for diagnostics)
715+
// Since !needsProcessing is true, we know ALL these conditions are false:
716+
// - syncMode.fullRebuild === false
717+
// - cachedPage exists
718+
// - no missing outputs
719+
// - path hasn't changed
720+
// - timestamp hasn't changed
721+
// If any "ERROR:" appears in logs, it indicates a logic bug in needsProcessing calculation
716722
let skipReason: string;
717723
if (syncMode.fullRebuild) {
718-
// This condition can't be reached if !needsProcessing, but for completeness
719-
skipReason = "full rebuild mode";
724+
// Should be unreachable (fullRebuild would make needsProcessing=true)
725+
skipReason = "🔴 ERROR: fullRebuild=true but !needsProcessing";
720726
} else if (!cachedPage) {
721-
skipReason = "not in cache (NEW PAGE - should not skip)";
727+
// Should be unreachable (!cachedPage would make needsProcessing=true)
728+
skipReason = "🔴 ERROR: not in cache but !needsProcessing";
722729
} else if (hasMissingOutputs(metadataCache, page.id)) {
723-
skipReason = "missing output files (should not skip)";
730+
// Should be unreachable (missing outputs would make needsProcessing=true)
731+
skipReason =
732+
"🔴 ERROR: missing output files but !needsProcessing";
724733
} else if (!cachedPage.outputPaths?.includes(filePath)) {
725-
skipReason = `path changed from [${cachedPage.outputPaths?.join(", ") || "none"}] to [${filePath}] (should not skip)`;
734+
// Should be unreachable (path change would make needsProcessing=true)
735+
skipReason = `🔴 ERROR: path changed [${cachedPage.outputPaths?.join(", ") || "none"}] → [${filePath}] but !needsProcessing`;
726736
} else {
727737
const notionTime = new Date(page.last_edited_time).getTime();
728738
const cachedTime = new Date(cachedPage.lastEdited).getTime();
729739
if (notionTime > cachedTime) {
730-
skipReason = `timestamp updated (${page.last_edited_time} > ${cachedPage.lastEdited}) (should not skip)`;
740+
// Should be unreachable (newer timestamp would make needsProcessing=true)
741+
skipReason = `🔴 ERROR: timestamp newer (${page.last_edited_time} > ${cachedPage.lastEdited}) but !needsProcessing`;
731742
} else {
743+
// This is the ONLY valid reason for !needsProcessing
732744
skipReason = `unchanged since ${cachedPage.lastEdited}`;
733745
}
734746
}

0 commit comments

Comments
 (0)