Skip to content

Commit 3585bf8

Browse files
committed
fix(owen): close two Codex-flagged holes in the rebrand (PR #246)
- CoreVendor's legacy ~/.ownsharp fallback trusted a bare .unpacked marker's existence keyed only by version. The CLI's own <Version> doesn't change every time the vendored core's content does - this rebrand's own SARIF-driver-name change is the concrete proof: same 0.1.0, different core content - so a machine with a pre-rebrand OwnSharp.Cli install's ~/.ownsharp/core/0.1.0/ cache could silently serve stale content (the old "Own.NET" driver name) to the new Owen.Cli 0.1.0 install. Reproduced locally: seeded a legacy cache with the pre-rebrand driver name and confirmed `owen check --format sarif` served it unchanged before this fix. Fixed by keying the marker on a SHA-256 fingerprint of the bundled ownlang/*.py files' names+content instead of bare existence - same location, both the new ~/.owen and legacy ~/.ownsharp fallback checks now only trust a cache whose marker matches the content this install actually bundles. Still the "simple fallback read, not a migration subsystem" shape requested - content-aware instead of version-only. Verified both directions locally: a stale-content legacy cache is now correctly rejected (fresh unpack to ~/.owen), and a legitimate matching-content legacy cache is still reused in place without an unnecessary ~/.owen copy. - CheckCommand.HasSupportedInput's directory walk used a plain SearchOption.AllDirectories, which throws on a locked/permission- denied subdirectory, while the extractor's own Expand() walk (OwnSharp.Extractor/Program.cs) uses EnumerationOptions { IgnoreInaccessible = true } specifically to tolerate that and keep scanning. The new preflight check could have crashed owen check on a directory tree the extractor itself would have handled fine. Fixed by using the identical EnumerationOptions the extractor already relies on. docs/notes/owen-public-facade.md updated to record both corrections in place, without erasing the original description of the intended (simpler, buggier) fallback shape.
1 parent 377a7b1 commit 3585bf8

3 files changed

Lines changed: 87 additions & 27 deletions

File tree

‎docs/notes/owen-public-facade.md‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -93,19 +93,37 @@ Per the guardrail this rebrand was scoped to: no mass rename.
9393
or a directory containing at least one `.cs` file anywhere under it)
9494
*before* running the extractor, and exits **4** with an explicit message
9595
if none do. This is a new, additive exit-code tier — it does not change
96-
any of the existing 0/1/`>=2`/3 contract.
96+
any of the existing 0/1/`>=2`/3 contract. **Correction (review, PR #246):**
97+
the directory walk in this preflight check now uses the same
98+
`EnumerationOptions { RecurseSubdirectories = true, IgnoreInaccessible =
99+
true }` the extractor's own `Expand()` walk uses (`OwnSharp.Extractor/
100+
Program.cs`) — the first version used a plain `SearchOption.AllDirectories`,
101+
which throws on a locked/permission-denied subdirectory instead of
102+
tolerating it and continuing, regressing a directory scan the extractor
103+
itself would have handled fine.
97104
- **`OWEN_PYTHON`** is the preferred env var; the legacy **`OWN_PYTHON`**
98105
name is still accepted as a temporary compatibility fallback (so existing
99106
internal use — CI, scripts, muscle memory — doesn't break outright) and
100107
prints a one-line deprecation note to stderr every time it's the variable
101108
actually used to resolve Python. `OWEN_PYTHON` takes priority when both
102109
are set.
103-
- **Cache directory** moved to `~/.owen/core/<version>/`. `CoreVendor` reads
104-
a plain existence check against the previous `~/.ownsharp/core/<version>/`
105-
location first — if that exact version was already unpacked there by an
106-
older install, it's reused in place rather than re-copied. This is a
107-
fallback read, not a migration subsystem: the old location is never
108-
written to, moved, or deleted by the new code.
110+
- **Cache directory** moved to `~/.owen/core/<version>/`. `CoreVendor` checks
111+
the previous `~/.ownsharp/core/<version>/` location first — if that exact
112+
version was already unpacked there by an older install, it's reused in
113+
place rather than re-copied. This is a fallback read, not a migration
114+
subsystem: the old location is never written to, moved, or deleted by the
115+
new code. **Correction (review, PR #246):** the first version of this
116+
fallback trusted a bare `.unpacked` marker file's *existence* — but the
117+
CLI's own `<Version>` doesn't change every time the vendored core's
118+
content does (this rebrand's own SARIF-driver-name change is the concrete
119+
proof: same `0.1.0`, different core content), so an existence-only check
120+
could silently reuse a stale, differently-vendored core from a machine
121+
that had run a pre-rebrand `OwnSharp.Cli` install. `CoreVendor` now writes
122+
a SHA-256 fingerprint of the bundled `ownlang/*.py` files as the marker's
123+
content (both locations) and only trusts a cache whose marker's hash
124+
matches the source this install would unpack right now — same "simple,
125+
not a migration subsystem" shape, just content-aware instead of
126+
version-only.
109127

110128
## Tests
111129

‎frontend/roslyn/OwnSharp.Cli/CheckCommand.cs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,17 @@ private static string RequireValue(string[] args, ref int i, string flag)
158158
return args[++i];
159159
}
160160

161+
// Mirrors the extractor's own Expand() walk (Program.cs): IgnoreInaccessible
162+
// tolerates a locked/permission-denied subdirectory mid-walk instead of
163+
// throwing. Without this, a preflight check using plain
164+
// SearchOption.AllDirectories could crash on a tree the extractor itself
165+
// would have scanned around just fine (review, PR #246).
166+
private static readonly EnumerationOptions DirectoryWalkOptions = new()
167+
{
168+
RecurseSubdirectories = true,
169+
IgnoreInaccessible = true,
170+
};
171+
161172
/// <summary>True if at least one of <paramref name="paths"/> resolves to
162173
/// something the currently included frontend can analyze: a file whose
163174
/// extension is in <see cref="SupportedExtensions"/>, or a directory that
@@ -172,7 +183,7 @@ private static bool HasSupportedInput(IReadOnlyList<string> paths, out string re
172183
{
173184
if (Directory.Exists(p))
174185
{
175-
if (Directory.EnumerateFiles(p, "*.cs", SearchOption.AllDirectories).Any())
186+
if (Directory.EnumerateFiles(p, "*.cs", DirectoryWalkOptions).Any())
176187
{
177188
reason = "";
178189
return true;
Lines changed: 50 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
using System.Security.Cryptography;
2+
using System.Text;
3+
14
namespace OwnSharp.Cli;
25

36
/// <summary>
@@ -8,9 +11,10 @@ namespace OwnSharp.Cli;
811
/// into the repository being analyzed. Cache root renamed to
912
/// <c>~/.owen/core/&lt;version&gt;/</c> for the Owen public facade (see
1013
/// docs/notes/owen-public-facade.md); a version already unpacked under the
11-
/// previous <c>~/.ownsharp/core/&lt;version&gt;/</c> location is reused as-is
12-
/// (a plain existence check, not a migration — the old location is never
13-
/// written to or deleted).
14+
/// previous <c>~/.ownsharp/core/&lt;version&gt;/</c> location is reused as-is —
15+
/// a plain fallback read, not a migration (the old location is never written
16+
/// to or deleted) — but ONLY when its content fingerprint still matches the
17+
/// source this install actually bundles (see <see cref="EnsureUnpacked"/>).
1418
/// </summary>
1519
internal static class CoreVendor
1620
{
@@ -22,37 +26,46 @@ internal static class CoreVendor
2226
/// </summary>
2327
public static string EnsureUnpacked()
2428
{
29+
var sourceOwnlang = Path.Combine(AppContext.BaseDirectory, "ownlang-core", "ownlang");
30+
if (!Directory.Exists(sourceOwnlang))
31+
{
32+
throw new InvalidOperationException(
33+
$"owen: vendored core not found at '{sourceOwnlang}' — a corrupt or " +
34+
"incomplete tool install. Try `dotnet tool uninstall --global Owen.Cli` " +
35+
"and reinstall.");
36+
}
37+
// A same-version marker alone does NOT prove the cached files match what
38+
// THIS install bundles (review, PR #246): the CLI's own <Version> can
39+
// stay unchanged across a content-only re-vendor (exactly what shipping
40+
// the Owen SARIF-driver-name rename without a version bump would do),
41+
// and the legacy ~/.ownsharp fallback below is a second location that
42+
// can hold same-version-but-different-content data from a genuinely
43+
// different install lineage (a pre-rebrand tool). A content fingerprint
44+
// closes both holes with the same simple mechanism, no migration
45+
// subsystem: trust a cache only when its marker's hash equals the hash
46+
// of the source this install would unpack right now.
47+
var fingerprint = Fingerprint(sourceOwnlang);
48+
2549
var userProfile = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile);
2650
var cacheRoot = Path.Combine(userProfile, ".owen", "core", ToolVersion.Current);
2751
var marker = Path.Combine(cacheRoot, ".unpacked");
2852

29-
if (File.Exists(marker))
53+
if (File.Exists(marker) && File.ReadAllText(marker) == fingerprint)
3054
{
3155
return cacheRoot;
3256
}
3357

3458
// Simple fallback read from the previous ~/.ownsharp location (same
35-
// version already unpacked there by an older install of this same
36-
// tool) — reuse it in place rather than re-copying, no migration
37-
// subsystem. Only a same-version marker counts: a different version
38-
// there is irrelevant (each version gets its own cache directory).
59+
// version, same content, already unpacked there by an older install)
60+
// — reuse it in place rather than re-copying.
3961
var legacyCacheRoot = Path.Combine(userProfile, ".ownsharp", "core", ToolVersion.Current);
4062
var legacyMarker = Path.Combine(legacyCacheRoot, ".unpacked");
41-
if (File.Exists(legacyMarker))
63+
if (File.Exists(legacyMarker) && File.ReadAllText(legacyMarker) == fingerprint)
4264
{
4365
return legacyCacheRoot;
4466
}
4567

4668
var destOwnlang = Path.Combine(cacheRoot, "ownlang");
47-
var sourceOwnlang = Path.Combine(AppContext.BaseDirectory, "ownlang-core", "ownlang");
48-
if (!Directory.Exists(sourceOwnlang))
49-
{
50-
throw new InvalidOperationException(
51-
$"owen: vendored core not found at '{sourceOwnlang}' — a corrupt or " +
52-
"incomplete tool install. Try `dotnet tool uninstall --global Owen.Cli` " +
53-
"and reinstall.");
54-
}
55-
5669
Directory.CreateDirectory(destOwnlang);
5770
foreach (var file in Directory.EnumerateFiles(sourceOwnlang, "*.py"))
5871
{
@@ -62,7 +75,25 @@ public static string EnsureUnpacked()
6275
// Write the marker LAST: an interrupted copy (killed process, full disk)
6376
// leaves no marker, so the next run redoes the unpack instead of running
6477
// against a half-written core.
65-
File.WriteAllText(marker, ToolVersion.Current);
78+
File.WriteAllText(marker, fingerprint);
6679
return cacheRoot;
6780
}
81+
82+
/// <summary>SHA-256 over every vendored <c>*.py</c> file's name and content,
83+
/// sorted by filename for determinism — a cheap, self-contained way to tell
84+
/// "this cache holds exactly this source" apart from "this cache exists and
85+
/// the version number happens to match".</summary>
86+
private static string Fingerprint(string sourceOwnlang)
87+
{
88+
using var sha = SHA256.Create();
89+
using var buffer = new MemoryStream();
90+
foreach (var file in Directory.EnumerateFiles(sourceOwnlang, "*.py").OrderBy(f => f, StringComparer.Ordinal))
91+
{
92+
var nameBytes = Encoding.UTF8.GetBytes(Path.GetFileName(file));
93+
buffer.Write(nameBytes);
94+
var contentBytes = File.ReadAllBytes(file);
95+
buffer.Write(contentBytes);
96+
}
97+
return Convert.ToHexString(sha.ComputeHash(buffer.ToArray()));
98+
}
6899
}

0 commit comments

Comments
 (0)