[#189] fix: bypass cache when --source/customUrl is provided

[rucka]

PR Information

PR Title: [#189] fix: bypass cache when --source/customUrl is provided
Story/Epic: #189
Type: Bug Fix
Priority: High (P0)
Assignee: rucka
Labels: cli

Summary

What Changed

ensureKBAvailable now bypasses cache when customUrl (--source) is explicitly provided, clearing the existing cache directory before re-installing from the specified source.

Why This Change

Previously, --source <url> was silently ignored when a cached KB already existed at ~/.pair/kb/<version>/. This caused silent data staleness for all --source users — the CLI returned the stale cache instead of downloading from the specified URL.

Story Context

User Story: As a CLI user or automation script wrapping pair install/pair update, I want --source <url> to always download and apply the specified KB archive, so that I can reliably switch between KB sources without manually deleting cache directories.

Acceptance Criteria addressed: AC-1 through AC-5

Changes Made

Implementation Details

• clearCachedKB utility: Added to cache-manager.ts — removes cached KB directory, safe no-op when cache doesn't exist
• Cache bypass logic: Modified ensureKBAvailable in kb-availability.ts — when customUrl is truthy, skip cache early-return and call clearCachedKB before proceeding to installation
• 7 new test cases: Bug reproduction (AC-1, AC-2), regression (AC-3 cache-hit preserved, AC-4 local path, AC-5 failed download), clearCachedKB unit tests

Files Changed

• Modified: apps/pair-cli/src/kb-manager/kb-availability.ts — conditional cache bypass
• Modified: apps/pair-cli/src/kb-manager/cache-manager.ts — clearCachedKB function
• Modified: apps/pair-cli/src/kb-manager/kb-availability.test.ts — 5 new test cases
• Modified: apps/pair-cli/src/kb-manager/cache-manager.test.ts — 2 new unit tests

Testing

Test Coverage

• Unit Tests: 7 new tests added, all passing (754 total)
• Regression: AC-3 confirms default cache-hit behavior unchanged
• Edge Cases: Same URL re-download, failed download, local paths

Test Results

Test Suite: ✅ 65/65 passing
Tests: ✅ 754/754 passing
Quality Gate: ✅ All passing (ts:check, test, lint, prettier, mdlint)

Testing Strategy

• Happy Path: Cache exists + remote --source → downloads new KB (AC-1, AC-2)
• Default preserved: No --source → returns cache immediately, zero HTTP calls (AC-3)
• Local paths: Cache exists + local --source → installs from local (AC-4)
• Error handling: Failed remote download behavior verified (AC-5)

Deployment Information

• No database migrations
• No feature flags
• No configuration changes
• Changeset deferred — will be added separately when ready to release

Rollback Plan

Revert the commit — existing behavior (always cache-hit) is restored.

Closes #189

[rucka]

Code Review — PR #194

Review Information

PR Number: #194
Author: rucka
Reviewer: AI (Claude)
Review Date: 2026-04-11
Story: #189 — KB remote --source ignored when cache dir already exists
Review Type: Bug Fix

Review Summary

Overall Assessment

• Approved with Comments — Minor issue noted (AC-5 discrepancy), can merge

Key Changes Summary

Minimal, well-scoped fix: ensureKBAvailable now branches on deps.customUrl — when present, clears the existing cache directory via new clearCachedKB utility before proceeding to installation. When absent, existing cache-hit behavior is preserved unchanged. 4 files changed (+188/−3), bulk of additions are tests.

Business Value Validation

Fix resolves silent data staleness for all --source users. Cache bypass is correctly scoped to explicit customUrl only (default behavior untouched).

Code Review Checklist

Functionality Review

• Requirements Met — AC-1 through AC-4 fully satisfied. AC-5 partially (see Major Issues)
• Business Logic — Branching logic is correct and minimal
• Integration — No external integration points affected
• Error Handling — Delegates to existing installer error handling
• Performance — No regressions; default path unchanged

Code Quality Assessment

• Readability — Clean if/else, self-documenting
• Maintainability — clearCachedKB properly encapsulated in cache-manager
• Naming — Clear function name, follows existing conventions
• Complexity — Minimal — one new function, one conditional branch

Technical Standards Compliance

• Style Guide — Prettier/ESLint applied (commit 0b1ccd5)
• Architecture — No new patterns or dependencies introduced
• Dependencies — Zero new dependencies

Security Review

No security concerns. No user input flows, no network changes, no secrets.

Testing Review

• Unit Tests — 2 new tests for clearCachedKB (exists + no-op)
• Integration Tests — 5 new tests for cache-bypass scenarios
• Edge Cases — Same URL, different URL, local path, failed download
• Mocking — Uses existing InMemoryFileSystemService + MockHttpClientService

Test Results: ✅ 754/754 passing
Quality Gate: ✅ All passing (ts:check, test, lint, prettier, mdlint)

Detailed Review Comments

Positive Feedback

• TDD workflow followed correctly: failing tests first (f8a11d8), then fix (0ddc286), then regression tests (af82d3e). Textbook bug-fix TDD.
• Minimal production diff: only +7/−3 lines in kb-availability.ts. Exactly the right size.
• Good test coverage: 7 new tests covering all 5 ACs with clear AC labels.
• clearCachedKB encapsulation: properly lives in cache-manager.ts alongside sibling utilities.
• Commit discipline: clean commit-per-task with conventional prefixes and story references.

Major Issues 🔍

• kb-availability.test.ts:554 — AC-5 test title vs. actual behavior mismatch. The test is titled "should preserve cache when remote customUrl download fails" but the comment on lines 579-581 acknowledges that clearCachedKB runs before the download, so cache IS removed on failure. This contradicts AC-5 which states: "the existing cached KB is NOT deleted." The story's Technical Risks section accepts this trade-off, but:

  1. The test title is misleading — it implies cache preservation that doesn't happen.
  2. AC-5 in the story body is unmet as written.

  Recommendation: Rename the test to accurately reflect behavior (e.g., "should throw when remote customUrl download fails (AC-5)") and add an explicit assertion that cache IS cleared (expect(fs.existsSync(expectedCachePath)).toBe(false)). Optionally update AC-5 wording in the story to match the accepted risk.

Minor Issues 💡

• kb-availability.test.ts:468,584 — vi.clearAllMocks() at start of individual tests is unnecessary when each test creates fresh mocks. Only the consoleLogSpy needs cleanup, which is already handled by .mockRestore(). Not blocking.

Risk Assessment

Risk	Impact	Probability	Mitigation
Cache deleted before download → user left with no KB on failure	Medium	Low	Acknowledged in story Technical Risks. Track as tech debt for atomic replacement.
Default cache behavior regression	High	Very Low	AC-3 test explicitly validates unchanged default path

Tech Debt

Item	Severity	Recommendation
Non-atomic cache replacement	Low	Future: download to temp dir, swap on success. Current approach acceptable per story risk analysis.

Adoption Compliance

Level 4 — No adoption skills available. No new dependencies, no concerns.

---

REVIEW COMPLETE:
├── PR:         #194: [#189] fix: bypass cache when --source/customUrl is provided
├── Story:      #189: KB remote --source ignored when cache dir already exists
├── Decision:   TECH-DEBT (approve, track AC-5 atomic replacement as debt)
├── Issues:     critical: 0 | major: 1 (AC-5 test naming) | minor: 1
├── Quality:    PASS — 754/754 tests, quality gate green
├── DoD:        4/5 dev criteria met (code review = this), changeset deferred
├── Adoption:   Level 4 — no adoption skills, no concerns
├── Debt:       1 item (non-atomic cache replacement)
└── Report:     Posted as PR comment

[rucka]

Review Fix Report — 41dd97c

All issues and tech debt from the code review have been resolved in a single commit.

Resolved Items

1. Tech Debt: Non-atomic cache replacement → Fixed

Before: clearCachedKB deleted the cache directory before the download attempt. If download failed, the user was left with no cached KB.

After: Backup/restore pattern ensures atomic replacement:

• backupCachedKB → renames cache to {path}.bak before install
• On success → removeBackupKB deletes the .bak backup
• On failure → restoreCachedKB renames .bak back to original path

AC-5 is now fully satisfied: failed downloads preserve the existing cache.

2. Major Issue: AC-5 test title mismatch → Fixed

• Old title: "should preserve cache when remote customUrl download fails (AC-5)" — misleading, cache was actually deleted
• New title: "should restore cache when remote customUrl download fails (AC-5)" — matches actual behavior
• Added assertions: expect(fs.existsSync(expectedCachePath)).toBe(true) + backup cleaned up

3. Minor Issue: Unnecessary vi.clearAllMocks() → Fixed

Removed from AC-1 and AC-2 tests — each test creates fresh mock instances, so global mock clearing was redundant.

Files Changed

File	Change
cache-manager.ts	Replace clearCachedKB with backupCachedKB, restoreCachedKB, removeBackupKB
kb-availability.ts	Backup/try/catch/restore pattern + extract installFromSource helper
cache-manager.test.ts	7 tests for backup/restore/remove (replaces 2 clearCachedKB tests)
kb-availability.test.ts	Fix AC-5 test title + assertions, remove vi.clearAllMocks()

Verification

Tests:       758/758 passing ✅
Quality Gate: All passing ✅ (ts:check, test, lint, prettier, mdlint)