fix(packaging): find_binary prefers freshest local build over stale bundled binary

[protostatis]

Found during the 24-site evaluation of #48's smart surface: an editable install (uv tool install -e ./python) on this repo silently resolved to _bin/unbrowser from May 1 — predating discover/extract_cards/page_model — even though target/release/unbrowser was freshly built. Every enrichment call failed with unknown method and SmartClient 'gracefully degraded' to empty cards/routes, which looked exactly like a code bug until traced.

Cause

find_binary() resolution was: env → bundled (unconditional) → target/debug → $PATH. The bundled path always won, and dev fallback never checked target/release.

Fix

Resolution is now: env → newest-by-mtime among {bundled, target/release, target/debug} → $PATH.

• Wheel installs: unchanged behavior (no dev checkout exists relative to site-packages).
• Source checkouts / editable installs: fresh local build wins; stale bundled binary no longer hijacks.
• Docstring corrected (it already claimed an order the code didn't implement).

Verification

New regression test test_find_binary_prefers_freshest_local_build builds a fake package+target layout and asserts mtime-based selection in all three orderings (release-fresh, debug-stale, debug-fresh). Env-override precedence covered too.

[protostatis]

Sky's Code Review

The fix correctly reprioritizes find_binary() resolution from an unconditional bundled-binary win to mtime-based selection among {bundled, target/release, target/debug}, which is the right behavior for editable installs where a stale shipped binary previously shadowed freshly built dev code. The logic is sound: candidates are collected with their mtimes, max() picks the freshest, env-override still wins first, and $PATH remains the last resort. The docstring is now accurate. Two minor concerns: (1) the dev binary name is hardcoded to "unbrowser" for both release and debug profiles instead of using _binary_name(), which breaks cross-platform selection (e.g. Windows .exe) — this was pre-existing for debug but is now also applied to release; (2) mtime ties (builds within the same second) resolve to the arbitrarily-first candidate rather than by a stable preference, which is a benign non-determinism. Neither blocks merge.

Verdict: Approve

Comments

• Ordering the candidate tuple as (mtime, label, path) and using max(candidates, key=lambda c: c[0]) is correct and readable; consider extracting the candidate-building into a small helper if this grows, but it's fine as-is for three sources.
• The regression test description is thorough (three orderings + env-override), which is exactly the coverage this mtime logic needs. Confirm the test also creates files with explicitly distinct mtimes (e.g. os.utime) rather than relying on natural filesystem timestamps, which can collide within the same second.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

python/unbrowser/__init__.py:86 — Dev binary name is hardcoded to "unbrowser" for both profiles rather than using _binary_name(). On Windows (or any platform where the binary carries an extension/suffix) this will never match the actual release/debug artifact. This was a latent issue in the old debug-only fallback, but the change now applies it to target/release as well — consider dev = dev_root / profile / _binary_name().

python/unbrowser/__init__.py:90 — max() on equal mtimes returns the first-encountered candidate, so a bundled binary and a dev build touched within the same timestamp granularity (notably on filesystems with coarse mtime, or quick rebuilds) resolve to whichever was appended first (bundled). If a deterministic fallback is desired, add a stable tiebreaker (e.g. prefer dev builds by ordering candidates dev-before-bundled, or sort by (mtime, path)). Low risk, but worth a comment if the intent is 'freshest dev build always wins'.

[protostatis]

Sky's Code Review

This PR fixes a real developer-experience bug: find_binary() unconditionally preferred the package-bundled binary, which in an editable/source checkout could be months stale and silently shadow a fresh cargo build (causing 'unknown method' enrichment failures). The fix correctly switches resolution to newest-by-mtime among {bundled, target/release, target/debug} before falling back to $PATH, preserves env-var override as the highest priority, and updates the docstring to match actual behavior. A regression test builds a fake package/target layout and asserts mtime ordering across all three cases. The logic is sound and the change is low-risk for wheel installs (unchanged behavior). A few minor nits around test hygiene and a pre-existing Windows path inconsistency remain, but nothing blocking.

Verdict: Comment

Comments

• mtime-based selection is a reasonable heuristic for the stated problem. One theoretical edge: if mtimes are exactly equal the tie is broken by max()'s first-encountered order, which is currently bundled-then-release-then-debug given the append order — that's a fine, deterministic fallback.
• The max(candidates, key=lambda c: c[0]) pattern is clean, but consider naming the tuple via a tiny dataclass or NamedTuple for readability now that three fields (mtime, label, path) are flowing through.
• No security, Docker, shell, or networking concerns — this is a pure-Python path-resolution change with good test coverage.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

The dev-build candidates hardcode the literal name "unbrowser" (also at line 87) while the bundled path uses _binary_name(). On Windows _binary_name() returns unbrowser.exe, so a fresh cargo build (which produces unbrowser.exe) would never match the dev path and the stale bundled .exe would keep winning. This is pre-existing for the debug path, but the new release path re-introduces it. Consider using _binary_name() for the dev candidates too.

[protostatis]

Minor TOCTOU: the file existence is checked with is_file() and then re-read via stat() a few lines later. If a candidate is removed between the two calls this raises. Low risk in practice (these are build artifacts), but wrapping the stat in try/except or using a single stat would be more robust — optional.

[protostatis]

The test does sys.path.insert(0, str(REPO / "python")) and imports unbrowser without restoring sys.path in a finally block (only unbrowser.__file__ is restored). This leaks global interpreter state into subsequent tests. Wrap the insert/import or the whole body in try/finally that pops the inserted path.

[protostatis]

All three addressed in daf4b46:

1. Windows name mismatch — real bug, good catch. All three candidates (bundled + release + debug) now derive from _binary_name(), so a fresh cargo build on Windows produces unbrowser.exe and wins over a stale bundled exe.

2. TOCTOU — replaced is_file()+stat() with a single stat() per candidate wrapped in except OSError: continue; vanished artifacts just drop out of the candidate list.

3. sys.path leak — the test now records its insert position and pops it in the finally alongside the __file__ restore.

Verified: 6/6 tests pass; live resolution still picks target/release/unbrowser over the bundled May binary.

[protostatis]

Sky's Code Review

The fix is well-reasoned and correctly addresses the root cause: the old resolver returned the bundled binary unconditionally, so an editable/source install silently used a months-stale _bin/unbrowser over a freshly built target/release/unbrowser. Reordering to mtime-based selection across {bundled, release, debug} with a single stat() (avoiding the is_file/stat TOCTOU) is a clean, correct implementation, and wheel-install behavior is preserved since no dev checkout exists relative to site-packages. The accompanying docstring now matches the code. The main concerns are test-side: the new test references an undefined REPO global, and it is not portable to Windows (hardcoded 'unbrowser' filenames plus #!/bin/sh shebang + chmod assume _binary_name() returns 'unbrowser' and a POSIX shell). These are test-quality issues rather than production bugs, so this is approvable with minor cleanup.

Verdict: Comment

Comments

• The production change itself (mtime-based selection with single stat) is solid and correctly fixes the stale-binary hijack; no security or reliability concerns in the implementation.
• Consider whether 'newest by mtime' is the right precedence for a dev build that may be older but deliberately chosen (e.g. an intentional cargo build --release vs a newer but stale target/debug). mtime is a reasonable heuristic here, but an explicit debug/release preference under an env flag would be more predictable than purely relying on filesystem timestamps.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

REPO is not defined anywhere in the visible diff. If it isn't a module-level global or fixture defined elsewhere in this file, this line raises NameError and the test won't run. Verify REPO exists or define it (e.g. from pathlib.Path(__file__).resolve().parents[2]).

[protostatis]

Duplicate import os inside the function: os was already imported at module scope (used for os.utime). The function-level import is redundant; drop it or move it to the module imports.

[protostatis]

max(candidates, key=lambda c: c[0]) is correct, but note it is silent about mtime ties and about a stale-but-present binary that is non-executable. The subsequent _checked_binary(path, source) re-validates executability, so a non-executable but newest candidate still errors out with a helpful message rather than falling back to an older usable build. Consider whether fallback-to-next-newest is desired; as-is it intentionally fails fast, which is the safer behavior.

[protostatis]

The fake binaries are written with a #!/bin/sh shebang and chmod 0o755, and filenames are hardcoded as unbrowser (via fake_bin(pkg / "_bin" / "unbrowser") etc.). On Windows, _binary_name() returns unbrowser.exe and there is no POSIX shebang/chmod, so both the fixture and find_binary()'s candidate paths will mismatch and this test will fail. If Windows CI is a target, gate these exact paths on _binary_name() and skip chmod/shebang on non-POSIX platforms.

[protostatis]

The test mutates the live unbrowser.__file__ and global sys.path, then restores them in finally. This is safe for a single test but can leak state if the assertion raises before real_file/real_path_pos are captured — they are currently captured before the try, so that's fine here. Consider wrapping the mutation in a unittest.mock.patch('unbrowser.__file__', ...) or a fixture to guarantee cleanup even if future edits reorder the setup.

[protostatis]

Second round addressed in f1cf865:

• REPO NameError — false alarm: it's a module-level global in this file (line 8, REPO = Path(__file__).resolve().parents[1]), pre-existing since feat(tooling-ux): minimal 3-tool surface with progressive discovery + escalation #48. No change needed.
• duplicate import os — the premise was off (os wasn't at module scope; the local import was load-bearing), but moved to module-level imports anyway, which is cleaner.
• mtime ties / non-executable newest — agreed with your conclusion: fail-fast with the helpful _checked_binary message is the safer contract than silently falling back to an older binary. Left as-is.
• Windows fixture mismatch — fixed: fixture filenames now use _binary_name() and chmod is POSIX-guarded. CI is ubuntu-only today, but the test no longer assumes that.
• mock.patch suggestion — acknowledged; setup order is already capture-before-mutate so the current form is safe, keeping the explicit version for readability.

6/6 green.

[protostatis]

Sky's Code Review

The patch corrects find_binary() resolution so that among the bundled binary and local dev builds (target/release, target/debug), the newest by mtime wins, instead of the bundled binary unconditionally taking precedence. This fixes a real bug where editable installs silently used a months-old bundled binary missing discover/extract_cards/page_model, causing 'unknown method' failures and silent degradation. The logic is reasonable: it collects candidates via a single stat() call (avoiding TOCTOU), picks max mtime, then falls back to $PATH, with UNBROWSER_BIN still overriding everything. The docstring and error message are updated to match actual behavior. The regression test constructs a fake package+target layout and asserts mtime-based selection across all orderings plus env-override default behavior. Overall the change is correct and well-tested; issues are minor/style only.

Verdict: Approve

Comments

• The fix correctly preserves wheel-install behavior (no dev checkout exists relative to site-packages, so only the bundled candidate is present) while making source/editable installs prefer a fresh local build — exactly the intended semantics.
• mtime-based 'newest' is a pragmatic heuristic and the right call here; a freshness comparison of st_mtime floats is fine even across filesystems as long as they share a clock, which bundled and local target/ binaries do (same checkout).
• The regression test would be stronger if it also asserted the env-override path (UNBROWSER_BIN) that the PR description claims is covered; as written only the three mtime ordering cases are asserted, not env precedence.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

python/unbrowser/__init__.py:88 — Minor: the comment "single stat avoids is_file/stat TOCTOU" is slightly misleading. A single path.stat() does avoid the classic is_file()→stat() check-then-act race, but the remaining check (stat succeeded → later _checked_binary opens it) is still inherently racy if the binary is replaced between stat and use. That's unavoidable and not a real concern for dev builds, so this is fine — just flagging the comment overstates the guarantee.

python/unbrowser/__init__.py:90 — Consider caching the resolution result (e.g. functools.lru_cache / module-level memo) if find_binary() is called on a hot path. Each MCP tool/enrichment call that resolves the binary will re-stat three paths; if this runs per-request it's harmless but slightly wasteful. Not a blocker.

tests/test_mcp_minimal.py:192 — The monkeypatch of unbrowser.file (a module global) is not re-entrant/thread-safe; if tests ever run in parallel this could clobber a concurrent test. It's restored in finally, so fine for the current single-threaded pytest run — just a note if parallelism is ever enabled.

tests/test_mcp_minimal.py:217 — Minor fragility: sys.path.pop(real_path_pos) assumes no earlier insertion shifted the index between insert and pop. Since this test controls its own sys.path.insert at the top and nothing else mutates sys.path in between, it's correct today, but relying on an index captured before other imports is a bit brittle.

[protostatis]

Sky's Code Review

The change fixes a real bug: find_binary() unconditionally preferred the package-bundled _bin/unbrowser over any local cargo build, so editable installs silently ran a months-old binary missing discover/extract_cards/page_model, causing every enrichment call to fail with unknown method and SmartClient to degrade to empty cards. The fix replaces the ordered fallback with an mtime-based selection across {bundled, target/release, target/debug}, keeping env-var override and $PATH fallback intact, and corrects the docstring. The logic is sound, the single-stat candidate collection avoids the is_file/stat TOCTOU, and the regression test exercises all three orderings plus env precedence. This is correct and well-tested; no production-blocking issues.

Verdict: Approve

Comments

• The test monkeypatches unbrowser.__file__ to a fake path and mutates sys.path; it correctly captures real_path_pos before re-assigning, but note sys.path.index(str(REPO / "python")) will return the first occurrence, which may not equal the insertion index 0 if that path was already present in sys.path. Cosmetic test-only edge case, not blocking.
• The except OSError around path.stat() also swallows permission errors on the parent directories, not just 'not present' — acceptable here since the fallback message lists all locations anyway.
• Not classic Docker/shell infrastructure, but the reliability intent (fresh build wins over stale bundled artifact in dev environments) is a very reasonable and correct behavior change.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

python/unbrowser/__init__.py:89 — Minor: the captured path.stat().st_mtime can differ from what _checked_binary later validates — if the freshest candidate is present but non-executable (e.g. built on a different arch, or a 0-byte corrupt artifact), _checked_binary raises instead of falling back to the next-newest candidate. Pre-existing pattern, but mtime-selection makes it slightly more likely to pick a 'new but unusable' binary over an 'old but working' one. Optional hardening: catch the specific failure from _checked_binary for non-env candidates and continue the search.

python/unbrowser/__init__.py:93 — Consider a deterministic tie-break when mtimes are equal (e.g. key=lambda c: (c[0], source_order))). max currently returns the first tuple on ties, which follows the tuple iteration order, so it's already stable — just noting the precedence is implicit rather than explicit.

python/unbrowser/__init__.py:84 — Subtle asymmetry: the bundled path uses the non-resolved Path(__file__).parent, while the dev paths use Path(__file__).resolve().parents[2]. Under a symlinked install the bundled root and dev root can resolve from different base paths. Works correctly for both the wheel and normal-checkout cases, but worth a comment documenting why only dev paths resolve.