Skip to content

Commit 9e2a758

Browse files
claudeclaude
committed
Owen facade: content-addressed cache, extractor-owned exit 4, case-insensitive extensions
Addresses review round 3 on PR #246 (three findings, all with committed CI regression tests, not just local verification): 1. Stale-cache pollution on a fingerprint mismatch: the round-2 fix overwrote the destination file-by-file and rewrote the marker to match the new source, but never removed a file the new source no longer had, so a removed-upstream module survived as an orphan while the marker claimed the (polluted) result was valid. CoreVendor now uses a content-addressed layout (~/.owen/core/<version>/<fingerprint>/) with no separate marker -- a fingerprint mismatch is a different path, never an in-place overwrite -- and publishes atomically (build into a temp sibling, verify the written destination's own fingerprint matches the source, then Directory.Move once). The legacy fallback now fingerprints the actual legacy destination content and only trusts it on an exact match, rejecting extra/missing/modified files instead of patching them. 2. HasSupportedInput's cheap directory check disagreed with the extractor's real expansion rules (bin/obj/generated/vendor skips), so a directory containing only skipped files passed the CLI preflight but produced zero real extractor inputs -- the exact silent-clean-scan bug exit 4 exists to prevent. Per review, the CLI no longer tries to duplicate the extractor's skip-list; the extractor itself now checks Expand(...).Distinct() and returns exit 4 with an explicit message when it's empty, and the CLI/own-check.sh's existing exit-code propagation carries that through unchanged. 3. SupportedExtensions now compares with StringComparer.OrdinalIgnoreCase so Foo.CS/App.CSPROJ are recognized like their lowercase spellings. ci.yml gains committed regression coverage (both OSes) for all of the above: matching/extra-file/modified-content legacy cache reuse and rejection, skipped-only and generated-only directories, an empty .csproj, a .sln with no projects, skipped-files-plus-real-source, an inaccessible subtree alongside a readable source, and an uppercase extension. docs/notes/owen-public-facade.md updated to describe the round-3 design. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSXTDuh1ZHdQc4QqmYwshw
1 parent 3585bf8 commit 9e2a758

6 files changed

Lines changed: 423 additions & 119 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2129,3 +2129,159 @@ jobs:
21292129
echo "$out" | grep -qi "OWN_PYTHON is deprecated" || { echo "FAIL: expected a deprecation note when OWN_PYTHON is used"; exit 1; }
21302130
echo "$out" | grep -q "OWN001" || { echo "FAIL: expected OWN001 in the output even via the legacy var"; exit 1; }
21312131
2132+
- name: Seed a same-content legacy cache and confirm it is reused (no ~/.owen copy)
2133+
# Review, PR #246 -- the fallback must trust DESTINATION content, not a
2134+
# marker's say-so. This is the legitimate case: exact byte-for-byte
2135+
# match with the just-packed core, reused in place.
2136+
run: |
2137+
rm -rf "$HOME/.owen" "$HOME/.ownsharp"
2138+
mkdir -p "$HOME/.ownsharp/core/0.1.0/ownlang"
2139+
cp ownlang/*.py "$HOME/.ownsharp/core/0.1.0/ownlang/"
2140+
driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python3 -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
2141+
[ "$driver" = "Owen" ] || { echo "FAIL: driver name '$driver' via the legacy cache"; exit 1; }
2142+
if [ -d "$HOME/.owen" ]; then
2143+
echo "FAIL: ~/.owen was created despite a legitimate matching legacy cache (fallback not reused in place)"; exit 1
2144+
fi
2145+
echo "OK: matching legacy cache reused in place, no unnecessary copy"
2146+
- name: Seed a legacy cache with an EXTRA (removed) file and confirm it is REJECTED
2147+
# The exact reproduction from review: an old cache holds a file the new
2148+
# source no longer has. A version-only marker would have missed this;
2149+
# the content fingerprint must not.
2150+
run: |
2151+
rm -rf "$HOME/.owen" "$HOME/.ownsharp"
2152+
mkdir -p "$HOME/.ownsharp/core/0.1.0/ownlang"
2153+
cp ownlang/*.py "$HOME/.ownsharp/core/0.1.0/ownlang/"
2154+
echo "# stale leftover module" > "$HOME/.ownsharp/core/0.1.0/ownlang/removed_module.py"
2155+
driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python3 -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
2156+
[ "$driver" = "Owen" ] || { echo "FAIL: driver name '$driver'"; exit 1; }
2157+
[ -d "$HOME/.owen" ] || { echo "FAIL: expected a fresh ~/.owen unpack (extra-file legacy cache should have been rejected)"; exit 1; }
2158+
if find "$HOME/.owen" -name "removed_module.py" | grep -q .; then
2159+
echo "FAIL: the stale extra file leaked into the fresh cache"; exit 1
2160+
fi
2161+
echo "OK: extra-file legacy cache correctly rejected, fresh cache is clean"
2162+
- name: Seed a legacy cache with MODIFIED content (same filenames, different bytes) and confirm rejection
2163+
# Same version, same file SET, different CONTENT -- the same-version
2164+
# class of bug this whole fingerprint mechanism exists to catch.
2165+
run: |
2166+
rm -rf "$HOME/.owen" "$HOME/.ownsharp"
2167+
mkdir -p "$HOME/.ownsharp/core/0.1.0/ownlang"
2168+
cp ownlang/*.py "$HOME/.ownsharp/core/0.1.0/ownlang/"
2169+
echo "# tampered" >> "$HOME/.ownsharp/core/0.1.0/ownlang/ownir.py"
2170+
driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python3 -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
2171+
[ "$driver" = "Owen" ] || { echo "FAIL: driver name '$driver'"; exit 1; }
2172+
[ -d "$HOME/.owen" ] || { echo "FAIL: expected a fresh ~/.owen unpack (modified-content legacy cache should have been rejected)"; exit 1; }
2173+
echo "OK: modified-content legacy cache correctly rejected"
2174+
- name: Directory containing only skipped files (bin/obj) -- exit 4, not a silent clean scan
2175+
run: |
2176+
rm -rf "$HOME/.owen" "$HOME/.ownsharp"
2177+
mkdir -p "$RUNNER_TEMP/owen-skip-only/bin" "$RUNNER_TEMP/owen-skip-only/obj"
2178+
cat > "$RUNNER_TEMP/owen-skip-only/bin/Ignored.cs" <<'EOF'
2179+
public class X { public void M() { var s = new System.IO.MemoryStream(); } }
2180+
EOF
2181+
cat > "$RUNNER_TEMP/owen-skip-only/obj/AlsoIgnored.cs" <<'EOF'
2182+
public class Y { public void M() { var s = new System.IO.MemoryStream(); } }
2183+
EOF
2184+
set +e
2185+
out=$(owen check "$RUNNER_TEMP/owen-skip-only" 2>&1)
2186+
rc=$?
2187+
set -e
2188+
echo "$out"
2189+
[ "$rc" -eq 4 ] || { echo "FAIL: expected exit 4 (bin/obj-only directory), got $rc"; exit 1; }
2190+
echo "$out" | grep -qi "no supported input" || { echo "FAIL: expected an explicit unsupported-input message"; exit 1; }
2191+
- name: Directory containing only generated files (*.g.cs) -- exit 4
2192+
run: |
2193+
mkdir -p "$RUNNER_TEMP/owen-generated-only"
2194+
cat > "$RUNNER_TEMP/owen-generated-only/Foo.g.cs" <<'EOF'
2195+
public class Gen { public void M() { var s = new System.IO.MemoryStream(); } }
2196+
EOF
2197+
set +e
2198+
out=$(owen check "$RUNNER_TEMP/owen-generated-only" 2>&1)
2199+
rc=$?
2200+
set -e
2201+
echo "$out"
2202+
[ "$rc" -eq 4 ] || { echo "FAIL: expected exit 4 (generated-only directory), got $rc"; exit 1; }
2203+
echo "$out" | grep -qi "no supported input" || { echo "FAIL: expected an explicit unsupported-input message"; exit 1; }
2204+
- name: Empty .csproj (no .cs files in its directory) -- exit 4
2205+
run: |
2206+
mkdir -p "$RUNNER_TEMP/owen-empty-proj"
2207+
cat > "$RUNNER_TEMP/owen-empty-proj/Empty.csproj" <<'EOF'
2208+
<Project Sdk="Microsoft.NET.Sdk">
2209+
<PropertyGroup><TargetFramework>net8.0</TargetFramework></PropertyGroup>
2210+
</Project>
2211+
EOF
2212+
set +e
2213+
out=$(owen check "$RUNNER_TEMP/owen-empty-proj/Empty.csproj" 2>&1)
2214+
rc=$?
2215+
set -e
2216+
echo "$out"
2217+
[ "$rc" -eq 4 ] || { echo "FAIL: expected exit 4 (empty .csproj), got $rc"; exit 1; }
2218+
- name: .sln with no usable projects -- exit 4
2219+
run: |
2220+
mkdir -p "$RUNNER_TEMP/owen-empty-sln"
2221+
cat > "$RUNNER_TEMP/owen-empty-sln/Empty.sln" <<'EOF'
2222+
Microsoft Visual Studio Solution File, Format Version 12.00
2223+
# no Project( lines at all
2224+
EOF
2225+
set +e
2226+
out=$(owen check "$RUNNER_TEMP/owen-empty-sln/Empty.sln" 2>&1)
2227+
rc=$?
2228+
set -e
2229+
echo "$out"
2230+
[ "$rc" -eq 4 ] || { echo "FAIL: expected exit 4 (.sln with no projects), got $rc"; exit 1; }
2231+
- name: Skipped files plus one real source -- the real one is still found, exit 1
2232+
run: |
2233+
mkdir -p "$RUNNER_TEMP/owen-mixed-skip/bin" "$RUNNER_TEMP/owen-mixed-skip/src"
2234+
cat > "$RUNNER_TEMP/owen-mixed-skip/bin/Ignored.cs" <<'EOF'
2235+
public class X { public void M() { var s = new System.IO.MemoryStream(); } }
2236+
EOF
2237+
cat > "$RUNNER_TEMP/owen-mixed-skip/src/Real.cs" <<'EOF'
2238+
using System.IO;
2239+
public class Leaky { public void Run() { var s = new MemoryStream(); s.WriteByte(1); } }
2240+
EOF
2241+
set +e
2242+
out=$(owen check "$RUNNER_TEMP/owen-mixed-skip" --fail-on-finding 2>&1)
2243+
rc=$?
2244+
set -e
2245+
echo "$out"
2246+
[ "$rc" -eq 1 ] || { echo "FAIL: expected exit 1 (real source found despite bin/ noise), got $rc"; exit 1; }
2247+
echo "$out" | grep -q "OWN001" || { echo "FAIL: expected OWN001"; exit 1; }
2248+
- name: Inaccessible subtree plus one readable source -- tolerated, real source still found
2249+
# On these hosted runners the job does not run as an all-powerful admin/
2250+
# root account, so chmod/icacls genuinely restricts access (unlike a
2251+
# root-based local sandbox, where this same scenario cannot be exercised).
2252+
run: |
2253+
mkdir -p "$RUNNER_TEMP/owen-mixed-locked/locked" "$RUNNER_TEMP/owen-mixed-locked/readable"
2254+
cat > "$RUNNER_TEMP/owen-mixed-locked/locked/Secret.cs" <<'EOF'
2255+
public class X { public void M() { var s = new System.IO.MemoryStream(); } }
2256+
EOF
2257+
cat > "$RUNNER_TEMP/owen-mixed-locked/readable/Real.cs" <<'EOF'
2258+
using System.IO;
2259+
public class Leaky { public void Run() { var s = new MemoryStream(); s.WriteByte(1); } }
2260+
EOF
2261+
chmod 000 "$RUNNER_TEMP/owen-mixed-locked/locked" 2>/dev/null || true
2262+
set +e
2263+
out=$(owen check "$RUNNER_TEMP/owen-mixed-locked" --fail-on-finding 2>&1)
2264+
rc=$?
2265+
set -e
2266+
chmod 755 "$RUNNER_TEMP/owen-mixed-locked/locked" 2>/dev/null || true
2267+
echo "$out"
2268+
if echo "$out" | grep -qi "UnauthorizedAccess\|Unhandled exception"; then
2269+
echo "FAIL: crashed on the locked subdirectory instead of tolerating it"; exit 1
2270+
fi
2271+
echo "$out" | grep -q "OWN001" || { echo "FAIL: expected the readable source's leak to still be found"; exit 1; }
2272+
- name: Uppercase extensions are accepted case-insensitively (Foo.CS)
2273+
run: |
2274+
mkdir -p "$RUNNER_TEMP/owen-uppercase"
2275+
cat > "$RUNNER_TEMP/owen-uppercase/Leak.CS" <<'EOF'
2276+
using System.IO;
2277+
public class Leaky { public void Run() { var s = new MemoryStream(); s.WriteByte(1); } }
2278+
EOF
2279+
set +e
2280+
out=$(owen check "$RUNNER_TEMP/owen-uppercase/Leak.CS" --fail-on-finding 2>&1)
2281+
rc=$?
2282+
set -e
2283+
echo "$out"
2284+
[ "$rc" -eq 1 ] || { echo "FAIL: expected exit 1 for uppercase .CS extension, got $rc"; exit 1; }
2285+
echo "$out" | grep -q "OWN001" || { echo "FAIL: expected OWN001"; exit 1; }
2286+
- name: Clean up cache state left by the edge-case tests above
2287+
run: rm -rf "$HOME/.owen" "$HOME/.ownsharp"

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

Lines changed: 102 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -87,43 +87,76 @@ Per the guardrail this rebrand was scoped to: no mass rename.
8787
the help text does not claim support that doesn't exist yet.
8888
- **Unsupported input fails explicitly.** Before this rebrand, pointing
8989
`check` at a `.ts` file or an empty/non-C# directory silently printed
90-
"0 findings" — indistinguishable from a genuinely clean C# scan.
91-
`CheckCommand.HasSupportedInput` now checks every given path resolves to
92-
something the included frontend can read (a `.cs`/`.csproj`/`.sln` file,
93-
or a directory containing at least one `.cs` file anywhere under it)
94-
*before* running the extractor, and exits **4** with an explicit message
95-
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. **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.
90+
"0 findings" — indistinguishable from a genuinely clean C# scan. The
91+
explicit-failure behavior now has two layers, split across the two
92+
components that each know a different half of the answer (review, PR
93+
#246 round 2): `CheckCommand.HasSupportedInput` does only the CHEAP,
94+
obvious check — an existing file with a recognized extension, or an
95+
existing directory — *before* running the extractor at all, catching a
96+
bare `.ts` file or a nonexistent path for free. `SupportedExtensions` is
97+
compared with `StringComparer.OrdinalIgnoreCase` (review, PR #246 round
98+
2: `Foo.CS`/`App.CSPROJ` are the same file kind as their lowercase
99+
spellings, on Windows/macOS filesystems and to MSBuild itself). The CLI
100+
does **not** also try to duplicate the extractor's directory-walk skip
101+
rules to predict whether a directory will actually yield anything — a
102+
directory containing only `bin/`, `obj/`, or generated (`.g.cs`,
103+
`.Designer.cs`, `.AssemblyInfo.cs`) files passed the CLI's cheap check
104+
fine but produced zero real extractor inputs, recreating the exact
105+
silent-clean-scan bug this exit tier exists to prevent. Duplicating the
106+
extractor's skip-list in the CLI would just drift from it over time
107+
(review, PR #246 round 2 explicit instruction: "do not duplicate the
108+
complete extractor expansion rules in the CLI"). Instead, the extractor
109+
itself is now the sole authority on "found nothing after expansion":
110+
`OwnSharp.Extractor/Program.cs` checks `Expand(rawInputs).Distinct()`
111+
immediately after computing it and returns a new exit code **4** with an
112+
explicit message if that list is empty — a `.csproj`/`.sln` with no
113+
usable source resolves the same way, since project/solution resolution
114+
is text/glob-based, not full MSBuild evaluation (see the `ProjectCsFiles`
115+
comment). `CheckCommand.RunAsync`'s existing `extractRc != 0` early
116+
return propagates that 4 unchanged, and `own-check.sh`'s existing `set
117+
-e` does the same for the script/Action path — no extra code needed on
118+
either side beyond the extractor's own check. This is a new, additive
119+
exit-code tier; it does not change any of the existing 0/1/`>=2`/3
120+
contract.
104121
- **`OWEN_PYTHON`** is the preferred env var; the legacy **`OWN_PYTHON`**
105122
name is still accepted as a temporary compatibility fallback (so existing
106123
internal use — CI, scripts, muscle memory — doesn't break outright) and
107124
prints a one-line deprecation note to stderr every time it's the variable
108125
actually used to resolve Python. `OWEN_PYTHON` takes priority when both
109126
are set.
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.
127+
- **Cache directory** moved to `~/.owen/core/<version>/<fingerprint>/`,
128+
where `fingerprint` is a SHA-256 over every vendored file's name and
129+
content (length-prefixed encoding, so e.g. name `"ab"` + content `"c"`
130+
can't collide with name `"a"` + content `"bc"` by bare concatenation).
131+
**Correction (review, PR #246 round 3):** the round-2 design (a
132+
version-keyed directory with a separate marker file holding a source
133+
fingerprint) had a gap the reviewer's reproduction demonstrated directly:
134+
on a fingerprint mismatch, the fix copied new files over the existing
135+
destination with `overwrite: true`, then rewrote the marker to match the
136+
new source — but a file the new source no longer has (e.g. a module
137+
deleted upstream) was never removed from the destination, so it survived
138+
as an orphan while the rewritten marker now claimed the (polluted)
139+
directory was valid. Round 3 removes the marker concept entirely and
140+
makes cache identity content-addressed: the fingerprint *is* the last
141+
path segment, so a content change is a different path, never an
142+
in-place overwrite of an existing one. Publication into a fresh path is
143+
atomic — build into a `.tmp-<guid>` sibling directory, recompute the
144+
fingerprint of what was actually written there, confirm it equals the
145+
source fingerprint, and only then `Directory.Move` it to the final
146+
fingerprint-named path — so a reader can only ever observe nothing, or a
147+
fully-written self-verified copy, never a partial one (a crash or a lost
148+
race with a concurrent `owen` process mid-copy just leaves a harmless
149+
orphaned temp directory that nothing consults). The previous flat
150+
`~/.ownsharp/core/<version>/` location (pre-rebrand layout, no
151+
fingerprint segment) is still checked first and used in place without
152+
copying — but only after fingerprinting what is *actually on disk* there
153+
right now and confirming it equals the current source fingerprint; a
154+
destination with extra, missing, or modified files fails that check and
155+
falls through to a fresh unpack instead of being trusted or patched.
156+
This is still a plain fallback *read*: the legacy location is never
157+
written to, moved, or deleted by this code — content-addressing didn't
158+
change that guardrail, only how "is this destination actually still
159+
correct" gets decided.
127160

128161
## Tests
129162

@@ -145,6 +178,44 @@ does **not** contain the literal clean-scan phrase `0 findings.`),
145178
by the driver-name change, not scope creep; they were caught by
146179
`python tests/run_tests.py` failing before the fix.
147180

181+
**Added in review round 3** — the reviewer's stated concern was that both
182+
bugs (stale-cache pollution, cheap-preflight/extractor disagreement) were
183+
"sufficiently non-obvious that a future cleanup could reintroduce [them]
184+
while the ordinary fresh-install smoke remains green," so both
185+
reproductions from the review became committed `ci.yml` steps rather than
186+
staying local manual verification. All of the below ran locally first
187+
(pack -> isolated-feed install -> exercise -> confirm exit code/output)
188+
before being encoded as assertions:
189+
- **Matching legacy cache is reused in place** — the legitimate case: a
190+
byte-for-byte-identical legacy `~/.ownsharp/...` unpack is used without
191+
copying (`~/.owen` is asserted to *not* get created).
192+
- **Extra (removed-upstream) file in the legacy cache is rejected** — the
193+
exact reproduction from the review: an old cache holds a file the new
194+
source no longer has. A fresh, clean `~/.owen` unpack must result, with
195+
the stale file absent from it.
196+
- **Modified-content legacy cache (same filenames, different bytes) is
197+
rejected** — same version, same file set, different content: the
198+
same-version class of bug the fingerprint exists to catch generally, not
199+
just the removed-file case.
200+
- **Directory containing only skipped files** (`bin/`/`obj/`) and
201+
**directory containing only generated files** (`*.g.cs`) both assert
202+
exit 4 with an explicit "no supported input" message — the CLI's cheap
203+
preflight passes these (a directory that exists), and only the
204+
extractor's own zero-expanded-input check catches them.
205+
- **Empty `.csproj`** (no `Compile` items resolve to any `.cs` file) and
206+
**`.sln` with no `Project(` lines** both assert exit 4 the same way.
207+
- **Skipped files alongside one real source file** asserts exit 1 with
208+
`OWN001` still found — proving the skip rules don't over-reject a
209+
directory that has genuine content alongside the noise.
210+
- **An inaccessible subtree alongside one readable source file** (`chmod
211+
000` on a subdirectory, meaningful on the hosted runners' non-root
212+
accounts unlike this session's root-based local sandbox) asserts no
213+
crash and the readable source's finding is still reported — the
214+
`EnumerationOptions { IgnoreInaccessible = true }` tolerance from review
215+
round 2, now with a real permission-denied subdirectory to exercise it.
216+
- **Uppercase extension (`Leak.CS`)** asserts exit 1 with `OWN001` found —
217+
the `StringComparer.OrdinalIgnoreCase` fix from review round 3.
218+
148219
## PR separation
149220

150221
This is PR 1 of 3 for the release-readiness work:

0 commit comments

Comments
 (0)