Owen facade: verify the CURRENT cache's actual content, not just its existence

Round-4 review finding: a hit at the content-addressed
~/.owen/core/<version>/<fingerprint>/ownlang path was trusted on
Directory.Exists alone, and the two concurrent-publisher race checks
around the atomic move made the same assumption. A path name is only a
claim, not proof -- a destination already sitting at the "right"
fingerprint could be modified, corrupted, or hand-assembled after
publication and would then be served as valid.

Every current-path hit now recomputes the fingerprint over what is
actually on disk and only trusts an exact match, the same standard the
legacy-cache fallback already held itself to. A mismatch quarantines the
invalid destination (atomic rename to a .invalid-<guid> sibling, then
best-effort recursive delete -- no in-place delete a concurrent reader
could observe mid-way) and falls through to the same temp-directory +
atomic-move rebuild a fresh unpack uses.

Added the round-4 regression to ci.yml: create the current cache, tamper
ownir.py and add a stale_module.py directly under the fingerprint-named
path, and assert the second run rejects it, rebuilds clean, and the
stale file is gone from whatever cache directory actually got used.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SSXTDuh1ZHdQc4QqmYwshw

Author: claude

.github/workflows/ci.yml

@@ -2292,5 +2292,29 @@ jobs:
           echo "$out"
           [ "$rc" -eq 1 ] || { echo "FAIL: expected exit 1 for uppercase .CS extension, got $rc"; exit 1; }
           echo "$out" | grep -q "OWN001" || { echo "FAIL: expected OWN001"; exit 1; }
+      - name: A tampered CURRENT (fingerprint-named) cache is rejected and rebuilt
+        # Review, PR #246 round 4 -- the earlier fixes verified a LEGACY cache's
+        # actual content, but a hit at the current ~/.owen/core/<version>/<fingerprint>/
+        # path itself was still trusted on existence alone. Reproduction: run once
+        # to create it, then tamper the file content directly under that exact
+        # fingerprint-named path (not the legacy location) and add a stale extra
+        # file, then confirm the second run rejects it, rebuilds cleanly, and the
+        # stale file is gone from whatever cache directory actually got used.
+        run: |
+          rm -rf "$HOME/.owen" "$HOME/.ownsharp"
+          owen check "$RUNNER_TEMP/owen-sample" --fail-on-finding > /dev/null 2>&1 || true
+          cache_dir=$(find "$HOME/.owen/core" -mindepth 2 -maxdepth 2 -type d)
+          [ -n "$cache_dir" ] || { echo "FAIL: first run did not create a current-cache directory"; exit 1; }
+          echo "# tampered" >> "$cache_dir/ownlang/ownir.py"
+          echo "# stale leftover module" > "$cache_dir/ownlang/stale_module.py"
+          driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
+          [ "$driver" = "Owen" ] || { echo "FAIL: driver name '$driver' after rebuild"; exit 1; }
+          if find "$HOME/.owen" -name "stale_module.py" | grep -q .; then
+            echo "FAIL: the stale extra file survived under a used cache directory"; exit 1
+          fi
+          if grep -q "# tampered" "$cache_dir/ownlang/ownir.py" 2>/dev/null; then
+            echo "FAIL: the tampered file content is still being served from the original path"; exit 1
+          fi
+          echo "OK: tampered current-cache destination rejected and rebuilt clean"
       - name: Clean up cache state left by the edge-case tests above
         run: rm -rf "$HOME/.owen" "$HOME/.ownsharp"

docs/notes/owen-public-facade.md

@@ -157,6 +157,23 @@ Per the guardrail this rebrand was scoped to: no mass rename.
   written to, moved, or deleted by this code — content-addressing didn't
   change that guardrail, only how "is this destination actually still
   correct" gets decided.
+  **Correction (review, PR #246 round 4):** round 3 still trusted a hit at
+  the CURRENT fingerprint-named path (`Directory.Exists(finalOwnlang)`) on
+  existence alone — the concurrent-publisher race checks did too. But a
+  path name is only ever a claim; nothing stopped a destination already
+  living at the "right" fingerprint from being modified, corrupted, or
+  hand-assembled after the fact, and that content would then be served as
+  if it were still the exact bytes the fingerprint names. Every current-
+  path hit (the initial check, and both concurrent-publisher checks around
+  the atomic move) now recomputes the fingerprint over what is actually on
+  disk there and only trusts it on an exact match, exactly like the legacy
+  fallback already did. A mismatch quarantines the invalid destination —
+  an atomic rename to a `.invalid-<guid>` sibling (so no concurrent reader
+  ever observes an in-place delete mid-way), then a best-effort recursive
+  delete of the renamed copy — and falls through to the same temp-
+  directory-plus-atomic-move rebuild a fresh unpack uses; since the source
+  didn't change, the rebuild lands back at the identical fingerprint-named
+  path, now holding a verified copy.
 
 ## Tests
 
@@ -215,6 +232,14 @@ before being encoded as assertions:
   round 2, now with a real permission-denied subdirectory to exercise it.
 - **Uppercase extension (`Leak.CS`)** asserts exit 1 with `OWN001` found —
   the `StringComparer.OrdinalIgnoreCase` fix from review round 3.
+- **A tampered CURRENT (fingerprint-named) cache is rejected and rebuilt**
+  (review round 4) — runs once to create `~/.owen/core/<version>/<fingerprint>/`,
+  tampers `ownlang/ownir.py`'s bytes and adds an `ownlang/stale_module.py`
+  directly under that exact path (not the legacy location the earlier
+  tests already covered), then asserts the second run's SARIF driver is
+  still `Owen`, `stale_module.py` is gone from whatever cache directory
+  actually got used, and `ownir.py` no longer contains the tampered
+  content anywhere under `~/.owen`.
 
 ## PR separation
 

frontend/roslyn/OwnSharp.Cli/CoreVendor.cs

@@ -35,6 +35,18 @@ namespace OwnSharp.Cli;
 /// falls through to a fresh unpack. This is still a plain fallback *read*,
 /// not a migration subsystem: the legacy location is never written to,
 /// moved, or deleted by this code.
+///
+/// A hit at the CURRENT (fingerprint-named) path is verified the same way
+/// (review, PR #246 round 4) — the path's name is only ever a claim, not
+/// proof; something could have modified, corrupted, or hand-assembled a
+/// directory that happens to sit at the "right" fingerprint since it was
+/// published. <see cref="Fingerprint"/> is recomputed over what is actually
+/// there on every hit (both the initial existence check and the
+/// concurrent-publisher race checks further down) and only trusted on an
+/// exact match; a mismatch quarantines the invalid destination (an atomic
+/// rename out of the way, then best-effort delete — never an in-place
+/// delete a concurrent reader could observe mid-way) and falls through to
+/// the same temp-directory + atomic-move rebuild used for a fresh unpack.
 /// </summary>
 internal static class CoreVendor
 {
@@ -62,27 +74,31 @@ public static string EnsureUnpacked()
         var finalRoot = Path.Combine(versionRoot, fingerprint);
         var finalOwnlang = Path.Combine(finalRoot, "ownlang");
 
-        // Content-addressed: existence at the fingerprint-named path IS proof of
-        // validity (see the atomic-publish note below) -- no separate marker to
-        // go stale or drift from the directory's actual contents.
+        // Content-addressed cache hit: verify the DESTINATION's actual content,
+        // not just its existence at the fingerprint-named path (review, PR #246
+        // round 4) -- a directory living under the "right" path is not proof it
+        // still holds the exact bytes that path name claims; only recomputing
+        // the fingerprint over what is actually there is. A mismatch means this
+        // path is invalid -- content-addressing has no business trusting it (it
+        // is not a "different, still-valid" cache the way a different
+        // fingerprint would be) -- quarantine it and fall through to rebuild.
         if (Directory.Exists(finalOwnlang))
         {
-            return finalRoot;
+            if (DestinationMatches(finalOwnlang, fingerprint))
+            {
+                return finalRoot;
+            }
+            QuarantineInvalidDestination(finalRoot);
         }
 
         // Legacy fallback: verify the LEGACY DESTINATION's actual content, not a
         // marker file's say-so (review, PR #246) -- a marker only proves "an
         // unpack happened here once", never that nothing since removed, added,
         // or modified a file in that directory.
         var legacyOwnlang = Path.Combine(userProfile, ".ownsharp", "core", ToolVersion.Current, "ownlang");
-        if (Directory.Exists(legacyOwnlang))
+        if (Directory.Exists(legacyOwnlang) && DestinationMatches(legacyOwnlang, fingerprint))
         {
-            var legacyFiles = SortedPyFiles(legacyOwnlang);
-            var legacyFingerprint = Fingerprint(legacyFiles);
-            if (legacyFingerprint == fingerprint)
-            {
-                return Path.Combine(userProfile, ".ownsharp", "core", ToolVersion.Current);
-            }
+            return Path.Combine(userProfile, ".ownsharp", "core", ToolVersion.Current);
         }
 
         // Fresh unpack: build into a temp sibling, verify the DESTINATION's own
@@ -111,10 +127,17 @@ public static string EnsureUnpacked()
 
             if (Directory.Exists(finalOwnlang))
             {
-                // Lost a race with a concurrent `owen` process that published the
-                // same fingerprint first -- their content is provably identical
-                // (same fingerprint), so just use it.
-                return finalRoot;
+                // Possibly lost a race with a concurrent `owen` process that
+                // published the same fingerprint first -- but only trust that if
+                // ITS destination actually verifies (review, PR #246 round 4).
+                // Existence proves nothing about a path anyone (or anything)
+                // could have written to since; "same fingerprint-named path" is
+                // not the same claim as "same, provably identical content".
+                if (DestinationMatches(finalOwnlang, fingerprint))
+                {
+                    return finalRoot;
+                }
+                QuarantineInvalidDestination(finalRoot);
             }
             try
             {
@@ -125,10 +148,16 @@ public static string EnsureUnpacked()
             {
                 // Narrower version of the same race (review, PR #246): a concurrent
                 // process created finalOwnlang between the check above and this
-                // Move. Same reasoning -- their content is provably identical
-                // (same fingerprint), so just use it instead of surfacing the
-                // IOException Move throws for an existing destination.
-                return finalRoot;
+                // Move. Same verification requirement as above -- only accept it
+                // if it actually matches; otherwise quarantine it and retry the
+                // move once with our own already-verified tempOwnlang copy.
+                if (DestinationMatches(finalOwnlang, fingerprint))
+                {
+                    return finalRoot;
+                }
+                QuarantineInvalidDestination(finalRoot);
+                Directory.CreateDirectory(finalRoot);
+                Directory.Move(tempOwnlang, finalOwnlang);
             }
             return finalRoot;
         }
@@ -144,6 +173,40 @@ public static string EnsureUnpacked()
     private static List<string> SortedPyFiles(string dir) =>
         Directory.EnumerateFiles(dir, "*.py").OrderBy(f => Path.GetFileName(f), StringComparer.Ordinal).ToList();
 
+    /// <summary>True only if every <c>.py</c> file actually on disk under
+    /// <paramref name="ownlangDir"/> right now fingerprints to
+    /// <paramref name="expectedFingerprint"/> (review, PR #246 round 4). This
+    /// is the sole source of truth for "is this destination still valid" --
+    /// a directory's location (even a content-addressed, fingerprint-named
+    /// one) is only ever a claim about what was published there once, never
+    /// proof of what is there now.</summary>
+    private static bool DestinationMatches(string ownlangDir, string expectedFingerprint) =>
+        Fingerprint(SortedPyFiles(ownlangDir)) == expectedFingerprint;
+
+    /// <summary>Moves an invalid cache destination out of the way of a rebuild
+    /// (review, PR #246 round 4). Renames first -- an atomic same-volume
+    /// rename can't be observed half-done the way an in-place recursive
+    /// delete could -- then best-effort deletes the renamed copy; a failure
+    /// there just leaves inert garbage that is never consulted again (the
+    /// quarantined name is never re-derived by <see cref="EnsureUnpacked"/>),
+    /// same reasoning as the orphaned-temp-directory cleanup above.</summary>
+    private static void QuarantineInvalidDestination(string invalidRoot)
+    {
+        var quarantined = $"{invalidRoot}.invalid-{Guid.NewGuid():N}";
+        try
+        {
+            Directory.Move(invalidRoot, quarantined);
+        }
+        catch (IOException)
+        {
+            // Lost a race with something else already handling this exact path
+            // (e.g. a concurrent process's own quarantine of the same invalid
+            // directory) -- nothing more to do; the caller re-checks fresh.
+            return;
+        }
+        try { Directory.Delete(quarantined, recursive: true); } catch (IOException) { /* best-effort cleanup */ }
+    }
+
     /// <summary>SHA-256 over every file's name and content, each explicitly
     /// length-prefixed (review, PR #246) so two different (name, content) sets
     /// can never hash identically by having their bytes merely concatenate the