Skip to content

Automatically provision a managed llama.cpp runtime during setup - #60

Merged
ledwindra merged 5 commits into
mainfrom
feature/issue-58-runtime-provisioning
Aug 2, 2026
Merged

ledwindra merged 5 commits into
mainfrom
feature/issue-58-runtime-provisioning

Conversation

@ledwindra

@ledwindra ledwindra commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Implements #58.

Summary

Adds fully managed llama.cpp runtime provisioning to econpapers setup so a fresh user no longer has to build llama.cpp, edit PATH, or locate an executable manually. Model acquisition remains manual and out of scope (issue #13 stays a separate, unapproved decision). Every other command (analyze, chat, bare econpapers, status) remains unconditionally network-free — the only network access anywhere in the application is this one explicit setup/bootstrap step.

Implementation plan was posted and reviewed on the issue before work started (see issue #58 comments); this PR follows that approved plan, amended for the five review points (atomic promotion via staging + content-addressed path, install receipt with full bundle checksums, independent runtime/model status, Python 3.10.12 floor for safe extraction, and hardened download/packaging).

What's new

  • domain/runtime_manifest.py + runtime_manifest_data.py — version-controlled, pinned llama.cpp b10199 release manifest for macOS arm64, macOS x86_64, Linux x86_64, and Windows x86_64 (covering Intel Macs too, not only the platforms this project's CI matrix runs). Archive size/SHA-256, and now full per-file bundle_member_checksums for every file in each pinned archive, are real, verified values computed directly from the actual GitHub release assets. Kept as a plain Python module (not JSON) so it's always included in built wheels/sdists with no separate packaging configuration.
  • services/platform_detection.py — pure platform/architecture detection, returning a typed "unsupported" result instead of raising.
  • protocols/runtime_provisioning.py — narrow Downloader/ArchiveExtractor protocols, kept separate so extraction-safety tests need no network and download-fault tests need no real archives.
  • adapters/runtime_downloader.py — stdlib-only (urllib.request) HTTPS downloader: HTTPS-only enforced on the initial URL and every redirect, a bounded redirect count, connect/read timeout, and an incremental byte cap enforced mid-stream (aborts as soon as the manifest's expected size is exceeded, not only after an unbounded download completes).
  • adapters/runtime_extractor.py — safe zip/tar extraction: rejects absolute paths, parent-directory traversal, symlink/hardlink members, and duplicate member names, using tarfile's PEP 706 filter="data" as defense in depth alongside explicit member validation.
  • domain/runtime_receipt.py + services/runtime_provisioning.py — the atomic install contract: download → verify archive size/SHA-256 → extract into a sibling staging directory → hash every extracted file and run the staged executable (--version --offline) to confirm real readiness, all while still staged → write a schema-versioned install receipt → promote via os.replace onto a content-addressed final path (<runtime_id>-<archive_sha256[:16]>/) that does not already exist. This is atomic and portable, and makes concurrent installs of the same pinned artifact race-safe by construction — colliding installs are byte-identical, so losing the promotion race just means adopting the winner instead of overwriting it. A corrupt existing directory at that path is evicted before a fresh install is promoted there, never overwritten in place.
  • setup_command.py--llama-cpp-path is now optional; omitting it triggers reuse-or-download-or-typed-offline-failure. Supplying it always bypasses managed provisioning entirely (never a download), unchanged CLI-override precedence. New --offline flag refuses any download.
  • status_command.py — replaces the old combined boolean with independent runtime (origin: managed/external/unknown × state: verified/missing/corrupt_or_mismatched/unsupported_platform/not_checked) and model (verified/missing/corrupt_or_mismatched/not_configured) classifications, so a missing/corrupt model is never conflated with a corrupt managed runtime or vice versa. Managed/external classification comes from a validated install receipt, not merely from the executable's directory location.
  • Python floor raised to >=3.10.12 for safe tarfile extraction (PEP 706), rather than hand-rolling the equivalent safety semantics for earlier 3.10 patch releases.
  • docs/managed-runtime-provisioning.md (new), plus README/architecture/roadmap updates.

Test plan

  • ruff check .
  • ruff format --check .
  • pytest (1239 passed)
  • New coverage: manifest/platform-selection unit tests; fake-downloader tests (success, oversized/aborted-mid-transfer, truncated, network failure, timeout, insecure-URL rejection, too-many-redirects, cleanup on every failure); safe-extraction tests (path traversal, absolute/drive-rooted paths, symlink escape, duplicate members, malformed archives, unsupported format); atomic-install fault-injection tests (staged-readiness failure never promotes, missing-executable-in-archive never promotes, archive checksum/size mismatch never promotes, corrupt receipt detected and triggers clean reinstall, tampered supporting library detected independently of the executable, offline never treats a corrupt managed install as ready, lost promotion race adopts the concurrent winner, idempotent reuse without redownload); setup-command tests (omitted-path triggers provisioning, explicit-path never calls the provisioner, offline flag threaded through, typed failures write nothing, managed-runtime identity threaded into the readiness check, provisioning failure never touches prior config); status-command tests (managed/external/unknown origin, all state combinations, independence between runtime and model classification); CLI parser tests (optional --llama-cpp-path, --offline flag, still-required model flags).

🤖 Generated with Claude Code

Adds fully managed llama.cpp runtime provisioning to `econpapers setup` so
a fresh user no longer has to build llama.cpp, edit PATH, or locate an
executable manually, while keeping model acquisition manual (issue #13
remains separate) and every other command unconditionally network-free.

- domain/runtime_manifest(_data).py: version-controlled, pinned llama.cpp
  b10199 release manifest (macOS arm64, Linux x86_64, Windows x86_64;
  real, verified archive size/SHA-256), as a plain Python module so it's
  always packaged.
- services/platform_detection.py: pure platform/arch detection with a
  typed unsupported-platform result instead of raising.
- protocols/runtime_provisioning.py + adapters/runtime_downloader.py
  (stdlib urllib, HTTPS-only, bounded redirects, timeout, incremental
  byte-cap) + adapters/runtime_extractor.py (safe zip/tar extraction:
  rejects traversal, absolute paths, symlinks, duplicate members).
- domain/runtime_receipt.py + services/runtime_provisioning.py: stage into
  a sibling directory, verify every bundle member's checksum and the
  executable's actual readiness while staged, then promote via os.replace
  onto a content-addressed final path that doesn't yet exist — atomic,
  portable, and race-safe by construction. A corrupt existing install at
  that path is evicted, never overwritten in place.
- setup_command.py: --llama-cpp-path is now optional (explicit path still
  always bypasses provisioning and never downloads); new --offline flag.
- status_command.py: independent runtime (origin x state) and model state
  classification, so a missing/corrupt model is never conflated with a
  corrupt managed runtime.
- Raises the Python floor to >=3.10.12 for safe tarfile extraction
  (PEP 706).
- docs/managed-runtime-provisioning.md, README, and architecture/roadmap
  updates.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@ledwindra ledwindra left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at head a1e5035b659648f66c837f74e34aecb4a80ee323: changes required before merge.

The implementation has the right overall layering, but the approved Issue #58 contract is not yet fully satisfied.

Blocking findings

  1. CI does not support the new Python floor. pyproject.toml now requires >=3.10.12, but the workflow matrix still requests 3.10. On current macOS and Windows runners, actions/setup-python resolves that to 3.10.11, so both jobs fail during package installation. Pin an available patch release satisfying the floor (for example 3.10.12 or newer) and require the full matrix to pass.

  2. econpapers status is not read-only. Its default runtime checker calls verify_executable_runs(), which adds execute bits with os.chmod() when they are missing. Merely running status can therefore modify a configured managed or external executable. Split staged-install preparation from read-only status validation (or add an explicit no-repair mode), and add a regression proving status preserves runtime/config/database bytes, mode, and timestamps.

  3. The manifest is not the single authoritative runtime identity after setup. The revised plan explicitly required the selected manifest identity to flow through setup, status, and later generation. This PR threads runtime_id/version_marker only into the immediate setup readiness check. LocalRuntimeModelConfig still persists neither field, ResolvedRuntimeModelConfig carries neither field, and LlamaCppConfig still hard-codes llama.cpp-b10199/10199. After restart, analyze/chat/interactive generation therefore fall back to adapter defaults rather than the identity actually installed or configured. Persist or deterministically resolve the runtime identity with backward-compatible schema-v1 handling, remove the duplicate hard-coded source of truth, and test restart-time generation configuration.

  4. Receipt verification is self-consistent but not bound to the selected manifest artifact. ensure_managed_runtime() finds the manifest-selected content-addressed path, but reuses any receipt there that passes its own declared member hashes. It does not compare the receipt's runtime ID, marker, platform, architecture, source URL, archive size/hash, or executable path with the selected ManagedRuntimeArtifact. status similarly accepts any self-consistent receipt beneath the runtime root. Verify exact receipt-to-manifest equality for managed reuse/status, validate the directory identity, enforce executable_sha256 == member_checksums[executable_relative_path], and reject undeclared extra regular files so injected adjacent libraries cannot escape bundle-integrity checks.

  5. Real provisioning integrity failures escape the typed setup boundary. The provisioning block in run_setup_command() catches RuntimeProvisioningError, DownloadError, and ExtractionError, but verify_local_file() and inspect_local_file() can raise VerificationError subtypes directly. A real archive checksum mismatch, size mismatch, or staged member inspection failure can therefore bypass the documented typed exit-code-2 path. Wrap these inside the provisioning service or catch them at setup, and add real-service setup tests for checksum/size/member failures and prior-config preservation.

  6. The approved artifact matrix is incomplete. The posted plan included macOS arm64 and x86_64, Linux x86_64, and Windows x86_64. The manifest contains no macOS x86_64 artifact, so an Intel Mac still requires a manual runtime path. Add the pinned macOS x86_64 asset and tests, or explicitly revise and re-approve the supported-platform contract before implementation.

  7. Archive duplicate protection is not cross-platform canonical. Duplicate detection compares raw member names only. Distinct entries such as a/b and a\\b, case-only variants on case-insensitive filesystems, or Windows trailing-dot/space and alternate-data-stream forms can resolve to the same extraction target and overwrite one another. Validate and deduplicate canonical target paths using Windows-safe rules before extraction, and add platform-independent regression fixtures for these collisions.

Non-blocking correction

  • _add_runtime_model_arguments(required=False) currently gives analyze/chat --llama-cpp-path help text saying omission auto-provisions a runtime. Those commands only fall back to durable configuration and must remain network-free. Restrict the auto-provision help suffix to setup.

Once these are fixed and CI is green on all six jobs, the PR can be re-reviewed.

…persistence, receipt binding, error wrapping, macOS x86_64, canonical extraction

- CI: pin Python to 3.10.12 (not bare "3.10", which resolved to 3.10.11 on
  some runners and failed the new >=3.10.12 floor).
- status is now strictly read-only: verify_executable_runs never chmods;
  permission repair moved into a install-only helper
  (_ensure_staged_executable_bit), applied only to freshly staged content.
- LocalRuntimeModelConfig gains optional runtime_id/runtime_version_marker
  (backward-compatible schema-v1 addition); threaded through
  ResolvedRuntimeModelConfig and a new shared
  config_resolution.build_llama_cpp_config_kwargs used by chat, analyze,
  and interactive-shell generator construction, so a managed-provisioned
  identity survives a process restart instead of falling back to
  LlamaCppConfig's hard-coded default.
- verify_managed_install now checks receipt-vs-manifest identity equality
  (runtime id, marker, platform, arch, source, archive size/hash,
  executable path) and rejects any undeclared extra regular file in an
  install directory; InstallReceipt itself now enforces
  executable_sha256 == member_checksums[executable_relative_path].
- VerificationError from archive/member inspection is now wrapped into
  StagedRuntimeVerificationError inside the provisioning service, so it
  can never escape the typed setup exit-code boundary.
- Added the pinned macOS x86_64 artifact (real, verified checksum) to the
  manifest, matching the originally approved platform matrix.
- Archive extraction duplicate/traversal checks are now canonical
  cross-platform: case-insensitive, backslash/forward-slash normalized,
  and reject NTFS alternate-data-stream syntax, reserved device names, and
  trailing dot/space components.
- Non-blocking: analyze/chat help text no longer claims omitting
  --llama-cpp-path auto-provisions a runtime (only setup does).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ledwindra

Copy link
Copy Markdown
Owner Author

Pushed a fix pass at 25a0a9e addressing all 7 blocking findings plus the non-blocking correction from the review at a1e5035.

  1. CI Python floor — matrix now pins "3.10.12" (exact patch) instead of bare "3.10", which was resolving to 3.10.11 on some runners and failing the new >=3.10.12 floor.
  2. Status read-onlyverify_executable_runs() no longer chmods anything; it's now a strictly read-only check. Permission repair moved into a new install-only helper (_ensure_staged_executable_bit), applied only to content the install step itself just staged. Added test_status_never_repairs_a_non_executable_runtime_file, which exercises the real default checker (not a fake) and asserts mode/mtime are unchanged.
  3. Runtime identity persistenceLocalRuntimeModelConfig gains optional runtime_id/runtime_version_marker (backward-compatible schema-v1 addition — old saved configs load fine with both None). Threaded through ResolvedRuntimeModelConfig and a new shared config_resolution.build_llama_cpp_config_kwargs, now used by chat/interactive-shell and analyze generator construction (previously each duplicated its own LlamaCppConfig(...) call). LlamaCppConfig's hard-coded default is only ever a fallback now, not a silent override of a real installed identity. Added restart-simulation tests using a deliberately non-default identity value to prove it's actually threaded through, not coincidentally matching the default.
  4. Receipt-to-manifest bindingverify_managed_install now takes an optional expected_artifact and checks exact equality (runtime id, version marker, platform, architecture, source asset, archive size/hash, executable path) against it; wired into both the setup reuse-check and status's classification (using the manifest-selected artifact for the current platform). Also now rejects any regular file in an install directory that isn't declared in the receipt's member_checksums. InstallReceipt itself now enforces executable_sha256 == member_checksums[executable_relative_path] at construction time.
  5. Typed-boundary escapeVerificationError from the archive-checksum check and from staged-member inspection is now caught inside ensure_managed_runtime and re-raised as StagedRuntimeVerificationError, so it can never bypass setup's typed exit-code-2 path. Added a setup-level test that goes through the real ensure_managed_runtime (fake downloader, real verification) to prove this end-to-end, plus updated the two existing runtime_provisioning-level tests that previously asserted the raw filesystem exception.
  6. macOS x86_64 artifact — added to the manifest with a real, verified checksum (downloaded and hashed the actual llama-b10199-bin-macos-x64.tar.gz release asset), matching the originally posted plan's platform matrix rather than only the CI-tested subset.
  7. Canonical duplicate/traversal protection — extraction now dedupes on a case-folded, separator-normalized key (catches a/b vs a\b, case-only collisions) and rejects NTFS alternate-data-stream syntax, reserved device names, and trailing dot/space components — all independent of whether the extractor itself runs on a case-sensitive host.
  • Non-blocking: analyze/chat help text no longer claims omitting --llama-cpp-path auto-provisions a runtime — only setup's help text does now.

ruff check, ruff format --check, and the full pytest suite (1228 passed, up from 1195) are all green.

@ledwindra ledwindra left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at head 25a0a9edf716e101f1a3fbe7c8bf252efd2ed469: changes are still required before merge.

The fix commit materially improves the PR: status no longer chmods files, managed identity is threaded into later generator construction, receipt fields are compared with the selected artifact, Intel macOS is present, checksum failures are wrapped, and archive-path canonicalization is stronger. However, the following blockers remain.

1. CI is still red: exact Python 3.10.12 is unavailable on current macOS/Windows runners

Both macos-latest / Python 3.10.12 and windows-latest / Python 3.10.12 fail in actions/setup-python before installation because that exact build is not available for those runner images. The four other jobs pass.

Keep requires-python = ">=3.10.12", but test an actually available 3.10 patch across all three operating systems. If exact-minimum coverage is desired, add a separate Linux-only 3.10.12 job and use a currently available later 3.10.x patch for the cross-platform matrix.

2. status confuses “recognized platform vocabulary” with “manifest-supported combination”

DetectedPlatform.is_supported means only that both raw values mapped to enums. Linux arm64 and Windows arm64 therefore return True even though the manifest has no artifact for those combinations.

Consequences in status_command.py:

  • with no config, such a machine is reported as not_checked, not unsupported_platform;
  • with a configured executable under the managed root, select_artifact_for_platform(...) returns None, which is then passed to verify_managed_install; expected_artifact=None disables manifest binding and can accept a merely self-consistent receipt on an unsupported combination.

Treat “no selected manifest artifact” as UNSUPPORTED_PLATFORM explicitly. Never call managed verification without the expected artifact when classifying a managed install.

3. Reuse does not re-check executable readiness and cannot repair a functionally broken managed install

In ensure_managed_runtime, an existing content-addressed directory returns immediately after receipt/member verification. The injected/default executable checker is not run on that reuse path or after adopting a concurrent winner.

A concrete failure: remove the executable bit from an otherwise checksum-valid managed install. Receipt verification passes, provisioning returns it, and the later setup readiness check fails. Even with downloads allowed, setup never reinstalls or repairs it because the provisioner already classified it as reusable. This contradicts the requirement that reuse occurs only after both pinned integrity and executable readiness succeed.

Run the executable checker before returning any reused/adopted install. If readiness fails, treat the install as unusable: reinstall when downloads are allowed, and return a typed offline failure otherwise. Add a regression using a checksum-valid but non-executable installed binary.

4. Durable identity is still not fully bound

There are three related gaps:

  • verify_managed_install(..., expected_artifact=...) compares receipt fields but never verifies that install_dir.name equals the expected content-addressed directory name. The prior review explicitly required directory-identity validation. status can therefore classify a valid bundle copied into an arbitrary direct child of the runtime root as managed.
  • status does not compare LocalRuntimeModelConfig.runtime_id / runtime_version_marker with the validated receipt. A tampered config can be reported verified because status runs the receipt marker, while analyze/chat later use the conflicting config marker and fail.
  • The documentation says an invalid receipt is insufficient to call the origin managed, but _classify_runtime returns RuntimeOrigin.MANAGED solely from path containment even when receipt validation fails. Either use UNKNOWN + CORRUPT_OR_MISMATCHED until provenance validates, or revise the origin semantics and documentation consistently.

Bind directory name, manifest artifact, receipt, configured executable path, and persisted runtime identity in one validation path, with focused mismatch tests.

5. The strict configuration schema was changed without a schema-version change

LocalRuntimeModelConfig is documented as an immutable, versioned schema, but this commit adds two serialized fields while continuing to write schema_version: 1. The new reader can load old v1 files, but an older strict v1 reader rejects newly written files as containing unknown fields. Thus schema version 1 no longer identifies one stable serialized contract and rollback compatibility breaks silently.

Use schema version 2 for newly written configs, while explicitly retaining a v1 read/migration path that supplies runtime_id=None and runtime_version_marker=None. “Read old v1” is the required compatibility; rewriting the meaning of v1 is not.

6. Installed member integrity is not anchored outside the mutable receipt

The approved revised plan called for the required bundle member set/checksums to be declared by the pinned artifact contract. The implementation instead computes all member hashes at install time and stores them only in receipt.json. A modified executable/library plus a correspondingly rewritten receipt passes verification, because the manifest anchors only the archive metadata and no expected member hashes are compared.

Either add the expected per-file bundle closure to the version-controlled manifest (preferred), or narrow the documented guarantee to accidental corruption rather than tamper detection. Also reject undeclared symlinks/special entries; actual_paths currently ignores symlinks entirely even though a fresh verified archive cannot legitimately create them.

7. Routine provisioning/download filesystem failures can still escape the typed setup boundary

run_setup_command catches provisioning/download/extraction domain errors, but several normal failures still propagate as raw exceptions:

  • runtime_dir.mkdir, staging-directory creation, receipt writing, and final promotion can raise raw OSError;
  • failure to remove a corrupt target is ignored and can later become a raw promotion error;
  • UrllibDownloader does not convert all read failures (for example http.client.IncompleteRead / other HTTPException cases) to DownloadError, and a partial destination may remain;
  • archive extraction can leak filesystem/runtime exceptions not covered by TarError or BadZipFile.

Map expected disk/network/archive failures to stable typed errors and prove cleanup/prior-config preservation at the setup boundary.

Documentation corrections

  • The README statement that setup “writes nothing on validation or readiness failure” is false once a verified managed runtime may remain installed after model/readiness or config-save failure. Say that the prior durable configuration is not replaced instead.
  • The PR description still lists only three platform artifacts, although macOS x86_64 was added.
  • The approved plan said cross-host redirects would be rejected, but the downloader permits any HTTPS redirect. Because official GitHub assets normally redirect to GitHub-owned asset hosts, define an explicit trusted-host policy or revise the approved contract rather than silently dropping it.

Once these items are fixed and the complete CI matrix is green, the PR can be reviewed again.

- Bind manifest-declared bundle_member_checksums to every extracted file
  (not just the executable), rejecting undeclared extra files in a
  staged/installed directory.
- Bump LocalRuntimeModelConfig schema to version 2 (runtime identity
  fields) with explicit backward-compatible reads of schema 1.
- Verify install-directory identity (content-addressed name) against the
  manifest-selected artifact before trusting a receipt as MANAGED;
  corrupt/mismatched provenance now reports UNKNOWN, never MANAGED.
- Distinguish "manifest recognizes this platform/arch" from "manifest has
  a pinned artifact for it" in status reporting.
- Require a real functional check (not just checksum validity) before
  reusing an installed runtime; fall back to reinstall or a typed offline
  failure otherwise.
- Map every raw OSError/http.client.HTTPException at the provisioning
  boundary to a typed exception so run_setup_command never leaks one.
- Restrict HTTPS redirects during download to an explicit GitHub-owned
  host allowlist instead of accepting any HTTPS host.
- Add macOS x86_64 to the pinned platform matrix.
- Correct README wording: setup leaves the prior durable config
  unreplaced on failure, but a verified managed runtime install may still
  remain on disk from that same attempt.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ledwindra

Copy link
Copy Markdown
Owner Author

Round 2 review fixes (head d2f9c83)

Addressed all findings from the re-review at 25a0a9e:

  1. Manifest-anchored bundle integritybundle_member_checksums on ManagedRuntimeArtifact is now the sole source of truth for every extracted file, not just the executable. runtime_manifest_data.py was regenerated with real, verified per-file SHA-256 checksums for all 4 pinned archives (macOS arm64: 43 files, macOS x86_64: 42, Linux x86_64: 52, Windows x86_64: 51), computed by downloading and hashing the actual GitHub release assets. _verify_staged_bundle_against_manifest verifies every declared entry and rejects any undeclared extra file found staged.
  2. Config schema versioningLOCAL_CONFIG_SCHEMA_VERSION bumped 1→2 for the new runtime_id/runtime_version_marker fields, with READABLE_LOCAL_CONFIG_SCHEMA_VERSIONS = {1, 2} so an old config file still loads with clear backward-compatible semantics rather than a confusing "unknown field" error.
  3. Directory-identity bindingverify_managed_install now also checks that the install directory's name matches the manifest-selected artifact's content-addressed name, not just the receipt contents. Any mismatch (or unpinned platform/arch) now classifies as RuntimeOrigin.UNKNOWN, never MANAGED — origin can only become MANAGED after full provenance (receipt + directory identity + manifest match + executable path + persisted config identity) checks pass.
  4. "Supported" vs. "pinned" — status reporting now distinguishes a recognized-but-unpinned platform/arch combination from a genuinely unsupported one, using select_artifact_for_platform(...) is not None as the true signal instead of DetectedPlatform.is_supported.
  5. Functional reuse check — reusing an existing managed install now requires an actual runtime execution check to pass, not just checksum/manifest validity. A checksum-valid-but-non-functional install (e.g. stripped exec bit) triggers reinstall when downloads are allowed, or a typed offline failure when they aren't.
  6. Typed exception boundary — every raw OSError/http.client.HTTPException at the provisioning boundary (directory creation, archive verification, receipt I/O, promotion) is now mapped to a typed exception (RuntimeInstallIOError, StagedRuntimeVerificationError) so nothing unhandled escapes to the CLI.
  7. Trusted redirect hosts — HTTPS redirects during download are now restricted to an explicit allowlist of GitHub-owned hosts (github.com, objects.githubusercontent.com, release-assets.githubusercontent.com, the last verified against the real pinned release URL), not merely required to be HTTPS.

Plus two documentation corrections: README no longer claims setup "writes nothing" on failure (a verified managed runtime install may still remain on disk even when the overall command fails later), and the PR description's platform matrix now lists all 4 pinned platforms including macOS x86_64.

All fixes have accompanying regression tests. ruff check ., ruff format --check ., and pytest (1239 passed) all green.

@ledwindra ledwindra left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at head d2f9c83dc0f9bf7823dd7af8e0bae23718d6bc7c: changes are still required before merge.

This commit fixes most of the prior round: pinned member checksums now exist, platform selection and directory/config identity are substantially improved, functional reuse is checked, common I/O failures are typed, redirects are host-restricted, and the README claim was corrected. Four blockers remain.

1. CI is still red on macOS and Windows

The new "3.10" + check-latest: true lane still resolves to Python 3.10.11 on both current macOS and Windows runners. Package installation then fails against requires-python = ">=3.10.12". The Linux 3.10.12 floor job and the remaining jobs pass, but the issue explicitly requires green Windows/macOS/Linux CI.

Use a cross-platform interpreter version that is actually available and satisfies the project floor (for example, a supported 3.11+ lane) while retaining the separate Linux-only 3.10.12 exact-floor job. Do not merge until the complete matrix is green.

2. Installed bundle verification is still not anchored to the manifest member closure

_verify_staged_bundle_against_manifest() correctly checks staged files against ManagedRuntimeArtifact.bundle_member_checksums. However, later reuse/status verification does not.

verify_managed_install(..., expected_artifact=...) calls _verify_receipt_matches_artifact(), but that helper compares runtime/archive identity fields only. It never compares:

  • receipt.member_checksums with expected_artifact.bundle_member_checksums; or
  • receipt.executable_sha256 with the manifest checksum for executable_relative_path.

It then hashes installed files against the mutable receipt. Therefore this sequence still passes:

  1. modify an installed supporting library;
  2. rewrite that member's checksum in receipt.json;
  3. call verify_managed_install(..., expected_artifact=artifact).

That contradicts the PR's stated tamper-detection guarantee. Require exact manifest-to-receipt member-map equality before trusting the receipt, and add a regression that modifies a file and rewrites the receipt; reuse and status must reject it.

3. A symlink or special entry at a declared member path can still be accepted

The installed-directory scan includes symlinks in actual_paths, but it only rejects paths that are undeclared. For a symlink occupying a declared path, inspect_local_file() resolves and hashes the target as a regular file. A declared executable/library can therefore be replaced by a symlink and still pass when the target bytes match the expected checksum.

A fresh safe extraction cannot legitimately produce symlinks or special files. Reject every installed member for which path.is_symlink() is true, and reject non-regular entries regardless of whether their relative path is declared. Add regressions for a declared executable symlink and a declared supporting-library symlink.

4. The v1→v2 configuration migration is not a stable serialized-schema contract

The writer now defaults to schema 2, but parsing/serialization does not branch by schema version:

  • schema 1 files containing the new v2 runtime fields are accepted;
  • schema 2 files omitting those fields are accepted;
  • loading a genuine schema 1 object and calling to_mapping() emits the v2 keys while retaining schema_version: 1;
  • the domain test named as a pre-Issue-58 migration test actually starts from a schema-2 mapping and merely deletes the two keys.

Define per-version serialized field sets. A genuine v1 file should be read through an explicit migration path that supplies None runtime identity and produces schema 2 on the next write. A v1 file with v2-only fields and a malformed/incomplete v2 file should be rejected. Update the stale LocalRuntimeModelConfig docstring and add actual read→resave migration tests.

After these fixes, rerun the full CI matrix and re-review the new head.

…nks, schema migration

- CI: replace the "3.10" + check-latest lane (still resolves to 3.10.11 on
  macOS/Windows runners, below the >=3.10.12 floor) with "3.11" for
  cross-platform coverage; the exact-floor Linux-only job is unchanged.
- verify_managed_install now compares the receipt's member_checksums to
  the manifest-selected artifact's bundle_member_checksums directly, not
  just identity fields — a receipt rewritten alongside a tampered file to
  stay "self-consistent" no longer passes.
- Reject a symlink occupying any declared bundle member path (executable
  or supporting file): a legitimate extraction never produces one, so its
  presence is itself the tamper signal, regardless of what its target
  resolves to.
- LocalRuntimeModelConfig.from_mapping now enforces per-schema-version
  serialized field sets: a schema-1 file must not contain runtime_id/
  runtime_version_marker at all (even null), and a schema-2 file must
  contain both keys. to_mapping() always writes the current schema
  version, so a genuine schema-1 read is transparently upgraded to
  schema 2 on its next write.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ledwindra

Copy link
Copy Markdown
Owner Author

Round 3 review fixes (head b54855f)

Addressed all 4 remaining blockers from the re-review at d2f9c83:

  1. CI red on macOS/Windows — the "3.10" + check-latest: true lane still resolved to 3.10.11 on both runner images, below the >=3.10.12 floor, so install failed. Replaced with "3.11" (always above the floor on every OS) for the cross-platform coverage lane; the separate ubuntu-only exact-3.10.12 floor job is unchanged. Full matrix (ubuntu/macos/windows × 3.11/3.14, plus the floor job) is green on this head: https://github.com/ledwindra/econ-paper-cli/actions/runs/30764473962
  2. Installed bundle verification not anchored to the manifest member closureverify_managed_install(..., expected_artifact=...) now compares receipt.member_checksums directly against expected_artifact.bundle_member_checksums (not just runtime/archive identity fields), so a receipt rewritten alongside a tampered file to stay "self-consistent" is rejected. New regression: tamper a supporting library and rewrite its receipt entry to match, then reuse/status must still reject it (test_tampered_file_with_rewritten_receipt_still_rejected_against_manifest).
  3. Symlink at a declared member path silently accepted — a symlink occupying any declared bundle member path (executable or supporting file) is now rejected outright before checksum inspection, since a legitimate extraction never produces one — regardless of whether its target's bytes happen to match the expected checksum. New regressions for both a declared executable symlink and a declared supporting-library symlink.
  4. v1→v2 config migration not a stable serialized-schema contractLocalRuntimeModelConfig.from_mapping now enforces per-schema-version serialized field sets: a schema-1 file must not contain runtime_id/runtime_version_marker at all (not even null), and a schema-2 file must contain both keys (a schema-2 file missing either is treated as malformed, not legacy). to_mapping() always writes the current schema version, so a genuine schema-1 read (preserving schema_version == 1 as actually read) is transparently upgraded to schema 2 the next time it's serialized. Updated the stale class docstring and replaced the previously-mislabeled "pre-issue-58" test (which actually started from a schema-2 mapping) with a real schema-1-mapping migration test, plus new tests for both invalid-field-presence cases.

All fixes have accompanying regression tests. ruff check ., ruff format --check ., and pytest (1246 passed) all green, and the full GitHub Actions CI matrix passed on this head.

@ledwindra ledwindra left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at head b54855f68dccc2b662d2fad6aa1d773e9d70fa5b: three of the four prior blockers are fixed, but one status-integrity bypass remains before merge.

What is now correct:

  • The full CI matrix is green: Windows/macOS/Linux run on Python 3.11 and 3.14, with the separate Ubuntu 3.10.12 exact-floor job also passing.
  • Installed receipt member checksums are now compared directly against the manifest-pinned bundle_member_checksums, so rewriting a tampered file and its receipt no longer defeats manifest-bound verification.
  • verify_managed_install() rejects a symlink occupying a declared executable or supporting-file path before checksum inspection.
  • The v1→v2 local-config contract now branches by declared schema version: genuine v1 files omit the new keys, malformed cross-version field sets are rejected, and subsequent serialization writes schema 2.

Remaining blocker: status can bypass managed verification for a symlinked executable

locate_managed_install_root() resolves executable_path before checking containment under the managed runtime directory. If a configured managed executable is replaced by a symlink whose target is outside the managed root, the resolved path is outside, so this helper returns None.

_classify_runtime() then takes the external-runtime branch instead of calling verify_managed_install(). If the injected readiness checker—or a real outside executable reporting the expected marker—passes, status reports RuntimeOrigin.EXTERNAL / RuntimeState.VERIFIED. The new direct verify_managed_install() symlink tests do not exercise this call-site bypass.

This contradicts the intended result of the prior blocker: a managed executable replaced by a symlink must be classified as corrupt/mismatched, not silently reclassified as a valid external runtime.

Please fix managed-origin discovery so it does not lose the lexical managed-install location by resolving the final executable symlink before classification. Also reject a symlinked install root itself. Add a status-level regression that:

  1. creates a valid managed install and durable managed config;
  2. replaces the configured executable with a symlink to an outside file whose readiness check succeeds;
  3. runs execute_status_command();
  4. asserts RuntimeOrigin.UNKNOWN and RuntimeState.CORRUPT_OR_MISMATCHED, never EXTERNAL / VERIFIED.

The reuse path itself is correctly routed through manifest-bound verification; the remaining defect is specifically the status origin-discovery path.

locate_managed_install_root() fully resolved the executable path before
checking containment under the managed runtime directory. A managed
executable replaced by a symlink to an outside file therefore resolved
outside the root, the helper returned None, and _classify_runtime() took
the external-runtime branch — skipping verify_managed_install() entirely
and reporting EXTERNAL/VERIFIED whenever the readiness check passed.

- Decide containment from the executable's lexical location, never from
  where symlinks along that path point. Benign platform canonicalization
  (macOS /var -> /private/var, Windows short names) is still absorbed by
  also testing a parent-resolved spelling, which never follows a symlink
  at the final component.
- Reject a symlinked install root in verify_managed_install(): promotion
  only ever creates a real directory, so reading a receipt through a
  symlinked root would validate whatever it points at.
- Add status-level regressions for both a symlinked configured executable
  (pointing at an outside file whose readiness check succeeds) and a
  symlinked install root; both assert UNKNOWN/CORRUPT_OR_MISMATCHED and
  explicitly never EXTERNAL/VERIFIED. Both fail without this fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ledwindra

Copy link
Copy Markdown
Owner Author

Round 4 fix (head 43bc31b): status origin-discovery bypass

Fixed the remaining blocker — locate_managed_install_root() resolving the final executable symlink before the containment check, which let _classify_runtime() take the external-runtime branch and skip managed verification entirely.

  • Containment is now decided lexically, never from where symlinks along the path point. Benign platform canonicalization (macOS /var/private/var, Windows short names) is still absorbed by also testing a parent-resolved spelling — that variant resolves only the parent directories and never follows a symlink at the final component, so it cannot reintroduce the escape.
  • A symlinked install root is now rejected in verify_managed_install(): promotion only ever creates a real directory, so reading a receipt through a symlinked root would validate whatever it points at rather than the managed install itself.

Two status-level regressions added, exactly as specified:

  1. test_status_never_reclassifies_symlinked_managed_executable_as_external — valid managed install + durable managed config, configured executable replaced by a symlink to an outside file whose readiness check succeeds (same bytes, so a checksum comparison alone would also have matched), execute_status_command() asserts RuntimeOrigin.UNKNOWN / RuntimeState.CORRUPT_OR_MISMATCHED and explicitly not EXTERNAL / VERIFIED.
  2. test_status_rejects_symlinked_managed_install_root — same assertions for a symlinked install root under the managed runtime directory.

Both were verified to fail against the previous head b54855f (reporting RuntimeOrigin.EXTERNAL) and pass with this fix, so they genuinely cover the call-site bypass rather than only the direct verify_managed_install() path.

ruff check ., ruff format --check ., and pytest (1248 passed) green locally; full CI matrix green on this head: https://github.com/ledwindra/econ-paper-cli/actions/runs/30765068742

@ledwindra ledwindra left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of commit 43bc31b against prior review 4839608718: approved; no remaining blocking findings at this head.

The status-integrity bypass is fixed at the correct boundary:

  • locate_managed_install_root() now determines managed containment from the executable's lexical location rather than fully resolving the final component. This preserves managed provenance when the configured executable itself has been replaced by an outside-pointing symlink, while the parent-resolved candidate still accommodates benign platform path canonicalization.
  • verify_managed_install() now rejects a symlinked install root before reading its receipt or members.
  • The new status-level regression uses an outside target whose readiness check succeeds and proves the result is UNKNOWN / CORRUPT_OR_MISMATCHED, never EXTERNAL / VERIFIED.
  • A second end-to-end regression covers a symlinked managed install root.
  • The change is a focused one-commit delta from b54855f, touching only runtime-origin verification and its status regressions.
  • GitHub Actions run 30765068742 is fully green across the Ubuntu 3.10.12 exact-floor job and the macOS/Windows/Linux Python 3.11 and 3.14 matrix.

The prior four round-3 findings are therefore resolved. PR #60 is approved for merge from code-review and test-coverage perspectives at exact head 43bc31b10f24983f901b52648ad60e5bb06ebd67.

Non-blocking metadata cleanup: update the PR description's stale pytest (1239 passed) count before or during merge if desired.

@ledwindra
ledwindra merged commit 945341e into main Aug 2, 2026
7 checks passed
@ledwindra
ledwindra deleted the feature/issue-58-runtime-provisioning branch August 8, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant