From 363b545dbfc8d2606ca28591ea4feb50593e15af Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 18 Aug 2026 11:44:45 -0700 Subject: [PATCH 1/7] fix: make the plugin generation store actually reclaimable (#4689) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No directory under ~/.autoskillit/plugin-generations/ has been reclaimable since the generation store was introduced in 0.10.933. Three defects compounded, and a fourth would have been triggered by fixing them. Routing. default_plugin_retirement_coordinator() registered the INSTALLED_PLUGIN owner at current_installed_plugin_root().parent — the legacy Claude plugin cache — while publish_generation() writes records under the generation store. sweep_due() dispatches by kind, so every generation record reached an owner whose managed_root could not contain it. try_reclaim() rejects an uncontained record WITHOUT removing it, so each record was re-rejected on every sweep forever. Observed: a record 10 days past its deadline, target genuinely superseded, never collected. Fixed by giving the generation store its own PLUGIN_GENERATION kind and an owner rooted at generation_store_root() — all versions, not one. The legacy tree keeps its own owner, since reconcile_install_artifacts() still enqueues it and the two roots are disjoint. Scope. _enqueue_prior_generation() was scoped managed_root=version_root, so it only queued superseded incarnations of the same version; a superseded VERSION was never queued at all. prune_stale_generations() replaces it, walking every version x incarnation in one pass. Self-protection. Each superseded version keeps its own per-version `current` symlink pointing at its own incarnation forever — nothing rewrites it on a newer publish. Honoring that meant every version vouched for itself. A plugin-level `current` selector is now authoritative once established, and only the live version's per-version selector is still honored. Codex coupling. ~/.codex/config.toml bakes absolute generation paths for all its hooks and re-reads them at launch without re-resolving; a per-session snapshot can outlive the version it names. Retiring old generations therefore breaks every safety guard baked into a stale config — this happened in practice. The bindingless resolver now prefers the version-independent selector, whose absolute path stays valid across version bumps. trusted_hash is unaffected: it hashes the dispatcher through the symlink, and _dispatch.py is byte-identical across versions. legacy_evidence. Migrated v1 evidence was never passed to try_reclaim, so it could not be reclaimed at all. try_promote_legacy_evidence() re-derives an exact identity and hands it to the normal queue rather than deleting anything itself. That path is only safe with a guard: the live legacy_evidence array records plugin-projections/.artifact-leases — the directory holding every running session's lease locks — as a "projection". An earlier prune enumerated that root with no dot-prefix filter. Anything that trusts the stored recognized_kind would delete the lease infrastructure out from under every live session. is_reclaimable_artifact_path() admits only non-hidden direct children of the managed root, and promotion re-derives eligibility from scratch instead of trusting persisted classification. Coverage. Nothing exercised the real coordinator against a real generation artifact: every existing fixture built the legacy cache shape, and the lifespan test mocks sweep_due entirely. That is why this shipped. tests/contracts/test_generation_retirement.py covers the routing regression, cross-version queueing, both selectors, the version-bump-survival of the pinned Codex path, and the .artifact-leases landmine. Refs #4689 --- src/autoskillit/cli/_plugin_artifact.py | 73 ++++- src/autoskillit/core/__init__.pyi | 7 + .../core/_plugin_artifact_identity.py | 30 +++ src/autoskillit/core/_plugin_cache.py | 79 ++++++ .../core/types/_type_plugin_source.py | 15 +- .../core/types/_type_protocols_workspace.py | 7 + .../execution/backends/_codex_hooks.py | 22 +- src/autoskillit/workspace/__init__.py | 4 + .../_generation_publication.py | 239 +++++++++++++--- .../workspace/_projection_cache.py | 12 + tests/arch/test_subpackage_isolation.py | 11 + tests/contracts/test_generation_retirement.py | 255 ++++++++++++++++++ 12 files changed, 701 insertions(+), 53 deletions(-) create mode 100644 tests/contracts/test_generation_retirement.py diff --git a/src/autoskillit/cli/_plugin_artifact.py b/src/autoskillit/cli/_plugin_artifact.py index 53a0f984c..6bfed4d83 100644 --- a/src/autoskillit/cli/_plugin_artifact.py +++ b/src/autoskillit/cli/_plugin_artifact.py @@ -9,7 +9,9 @@ from typing import TYPE_CHECKING from autoskillit.core import ( + _AUTOSKILLIT_PLUGIN_KEY, ArtifactLease, + LegacyRetiringEvidence, PluginArtifactIdentity, PluginArtifactKind, PluginArtifactLifecycleLease, @@ -25,6 +27,7 @@ RetiringCacheReadResult, RetiringCacheState, due_retiring_records, + generation_store_root, get_logger, installed_plugin_artifact_lease_path, installed_plugin_artifact_manifest_path, @@ -422,6 +425,27 @@ def try_reclaim( ) -> RetirementOutcome: return self._retirement.try_reclaim(record, now) + def try_promote_legacy_evidence( + self, + evidence: LegacyRetiringEvidence, + now: datetime, + ) -> RetirementOutcome: + return self._retirement.try_promote_legacy_evidence( + evidence, + now, + identity_for_path=self.identity_for_path, + ) + + def identity_for_path(self, managed_path: Path) -> PluginArtifactIdentity: + """Validate and return the exact current identity at a managed path.""" + return _read_and_validate_identity( + managed_path, + expected_semantic_key=installed_plugin_semantic_key( + _AUTOSKILLIT_PLUGIN_KEY, + Path(managed_path).name, + ), + ) + class DefaultPluginRetirementCoordinator: """Cross-kind retirement dispatcher used by startup and explicit sweeps.""" @@ -431,15 +455,14 @@ def __init__( *, projection_owner: PluginArtifactRetirementOwner, installed_owner: PluginArtifactRetirementOwner, + generation_owner: PluginArtifactRetirementOwner, ) -> None: - self._owners = { + self._owners: dict[PluginArtifactKind, PluginArtifactRetirementOwner] = { PluginArtifactKind.PROJECTION: projection_owner, PluginArtifactKind.INSTALLED_PLUGIN: installed_owner, + PluginArtifactKind.PLUGIN_GENERATION: generation_owner, } - self._managed_roots = { - PluginArtifactKind.PROJECTION: projection_owner.managed_root, - PluginArtifactKind.INSTALLED_PLUGIN: installed_owner.managed_root, - } + self._managed_roots = {kind: owner.managed_root for kind, owner in self._owners.items()} def migrate_legacy_cache(self) -> RetiringCacheReadResult: """Upgrade path-only retirement evidence before exact-v2 mutations.""" @@ -466,24 +489,52 @@ def sweep_due(self, now: datetime) -> tuple[RetirementOutcome, ...]: stacklevel=2, ) return () - outcomes = [RetirementOutcome.LEGACY_EVIDENCE for _item in state.legacy_evidence] + outcomes: list[RetirementOutcome] = [] + for evidence in state.legacy_evidence: + owner = ( + self._owners.get(evidence.recognized_kind) if evidence.recognized_kind else None + ) + if owner is None: + outcomes.append(RetirementOutcome.LEGACY_EVIDENCE) + continue + outcomes.append(owner.try_promote_legacy_evidence(evidence, now)) + # Re-read after promotion so newly minted records reclaim in this pass. for record in due_retiring_records(now): - owner = self._owners[record.artifact_kind] - outcomes.append(owner.try_reclaim(record, now)) + record_owner = self._owners.get(record.artifact_kind) + if record_owner is None: + outcomes.append(RetirementOutcome.REJECTED_IDENTITY) + continue + outcomes.append(record_owner.try_reclaim(record, now)) return tuple(outcomes) def default_plugin_retirement_coordinator() -> DefaultPluginRetirementCoordinator: - """Compose installed and projected owners without leaking CLI into server lifespan.""" - from autoskillit.workspace import ProjectedPluginRetirementOwner + """Compose every artifact-kind owner over its own disjoint managed root. + + The generation store and the legacy Claude plugin cache are separate trees. + A single owner cannot serve both: ``try_reclaim`` rejects any record outside + its ``managed_root`` and never removes it, so a misrouted record is + re-rejected on every sweep for the life of the install. + """ + from autoskillit.workspace import ( + GenerationArtifactRetirementOwner, + ProjectedPluginRetirementOwner, + ) - projection_root = Path.home() / ".autoskillit" / "plugin-projections" + home = Path.home() + projection_root = home / ".autoskillit" / "plugin-projections" installed_owner = InstalledPluginArtifactRetirementOwner( current_installed_plugin_root().parent ) + generation_owner = GenerationArtifactRetirementOwner( + generation_store_root(home, _AUTOSKILLIT_PLUGIN_KEY), + home=home, + plugin_ref=_AUTOSKILLIT_PLUGIN_KEY, + ) return DefaultPluginRetirementCoordinator( projection_owner=ProjectedPluginRetirementOwner(projection_root), installed_owner=installed_owner, + generation_owner=generation_owner, ) diff --git a/src/autoskillit/core/__init__.pyi b/src/autoskillit/core/__init__.pyi index 55d771a45..0e787d240 100644 --- a/src/autoskillit/core/__init__.pyi +++ b/src/autoskillit/core/__init__.pyi @@ -33,6 +33,9 @@ from ._plugin_artifact_identity import ( from ._plugin_artifact_identity import ( generation_artifact_root as generation_artifact_root, ) +from ._plugin_artifact_identity import ( + generation_plugin_selector_path as generation_plugin_selector_path, +) from ._plugin_artifact_identity import ( generation_selector_path as generation_selector_path, ) @@ -64,12 +67,16 @@ from ._plugin_artifact_identity import ( from ._plugin_artifact_identity import ( resolve_current_generation as resolve_current_generation, ) +from ._plugin_artifact_identity import ( + resolve_current_generation_for_plugin as resolve_current_generation_for_plugin, +) from ._plugin_cache import KitchenProcessIdentity as KitchenProcessIdentity from ._plugin_cache import PluginArtifactRetirementEngine as PluginArtifactRetirementEngine from ._plugin_cache import _InstallLock as _InstallLock from ._plugin_cache import any_kitchen_open as any_kitchen_open from ._plugin_cache import append_retiring_record as append_retiring_record from ._plugin_cache import due_retiring_records as due_retiring_records +from ._plugin_cache import is_reclaimable_artifact_path as is_reclaimable_artifact_path from ._plugin_cache import kitchen_entry_alive as kitchen_entry_alive from ._plugin_cache import migrate_retiring_cache_v1 as migrate_retiring_cache_v1 from ._plugin_cache import read_active_kitchens_registry as read_active_kitchens_registry diff --git a/src/autoskillit/core/_plugin_artifact_identity.py b/src/autoskillit/core/_plugin_artifact_identity.py index f2e2b2869..5b9ea947a 100644 --- a/src/autoskillit/core/_plugin_artifact_identity.py +++ b/src/autoskillit/core/_plugin_artifact_identity.py @@ -78,6 +78,36 @@ def resolve_current_generation(home: Path, plugin_ref: str, version: str) -> Pat return target if target.is_dir() and target.parent == version_root else None +def generation_plugin_selector_path(home: Path, plugin_ref: str) -> Path: + """Return the version-independent ``current`` symlink for a plugin. + + Lives one level above the per-version selector, so a consumer that must + bake an absolute path into persisted config (the Codex hooks config) can + name a path that survives version bumps instead of pinning one exact + incarnation that retirement will later reclaim. + """ + return generation_store_root(home, plugin_ref) / "current" + + +def resolve_current_generation_for_plugin(home: Path, plugin_ref: str) -> Path | None: + """Resolve the version-independent selector to the active generation. + + Returns ``None`` when no selector exists or the target is not a generation + directory directly beneath a version directory of this plugin's store. + """ + selector = generation_plugin_selector_path(home, plugin_ref) + if not selector.is_symlink(): + return None + try: + store_root = selector.parent.resolve(strict=True) + target = selector.resolve(strict=True) + except OSError: + return None + if not target.is_dir(): + return None + return target if target.parent.parent == store_root else None + + def installed_plugin_artifact_manifest_path(managed_root: Path) -> Path: """Return the stable external manifest for one installed plugin root.""" root = Path(managed_root) diff --git a/src/autoskillit/core/_plugin_cache.py b/src/autoskillit/core/_plugin_cache.py index 4ade41c8c..25b831a6e 100644 --- a/src/autoskillit/core/_plugin_cache.py +++ b/src/autoskillit/core/_plugin_cache.py @@ -294,6 +294,25 @@ def _legacy_record_id(version: str, path: str, retired_at: str) -> str: return hashlib.sha256(payload).hexdigest() +def is_reclaimable_artifact_path(path: Path, managed_root: Path) -> bool: + """Return whether *path* may ever be treated as a retirable incarnation. + + Only a direct, non-hidden child of *managed_root* qualifies. Dot-prefixed + entries are managed infrastructure, never artifacts — most importantly + ``plugin-projections/.artifact-leases``, the directory holding the lock + files every live session's inherited reader lease is held on. An earlier + ``prune_stale_projections`` enumerated that root without a dot-prefix filter + and queued the lease directory alongside real projections, so persisted + evidence recording it as a ``projection`` already exists on disk. Reclaiming + it would delete the lease infrastructure out from under every running + session. + + Nested descendants are excluded too: an artifact is exactly one level deep, + so anything deeper is a component of an artifact rather than an artifact. + """ + return not path.name.startswith(".") and path.parent == managed_root + + def _classify_legacy_path( path: str, managed_roots: Mapping[PluginArtifactKind, Path], @@ -313,6 +332,8 @@ def _classify_legacy_path( except OSError: continue if location != managed_root and location.is_relative_to(managed_root): + if not is_reclaimable_artifact_path(location, managed_root): + return None, "legacy path is managed infrastructure, not an artifact incarnation" return kind, None return None, "legacy path is outside known managed roots" @@ -702,6 +723,64 @@ def try_reclaim(self, record: RetiringArtifactRecord, now: datetime) -> Retireme finally: writer.close_preserving() + def try_promote_legacy_evidence( + self, + evidence: LegacyRetiringEvidence, + now: datetime, + *, + identity_for_path: Callable[[Path], PluginArtifactIdentity], + ) -> RetirementOutcome: + """Promote path-only legacy evidence into an exact v2 record. + + Migrated v1 evidence carries no incarnation or digest, so it can never + authorize deletion by itself. This re-derives an exact identity from + disk and hands it to the normal queue; the artifact is then removed by + ``try_reclaim`` under the same lease and identity checks as any other + record. Nothing is deleted here. + + Eligibility is re-derived from scratch rather than trusting the stored + ``recognized_kind``: that field is exactly what a past classification + bug got wrong for ``.artifact-leases``, and evidence already persisted + with a wrong kind must not become authority now. + """ + if evidence.recognized_kind is not self.artifact_kind: + return RetirementOutcome.LEGACY_EVIDENCE + try: + path = destination_location(Path(evidence.path)) + except (OSError, ValueError): + return RetirementOutcome.LEGACY_EVIDENCE + if not is_reclaimable_artifact_path(path, self.managed_root): + return RetirementOutcome.LEGACY_EVIDENCE + if not self.contains(path): + return RetirementOutcome.LEGACY_EVIDENCE + if not path.exists() and not path.is_symlink(): + # Nothing left to protect; drop the bookkeeping without any I/O. + remove_retiring_records((evidence.record_id,)) + return RetirementOutcome.RECORD_REMOVED + try: + writer = ArtifactLease.acquire_exclusive(self._lease_path(path), blocking=False) + except ArtifactLeaseContention: + return RetirementOutcome.DEFERRED_CONTENDED + except (OSError, RuntimeError): + return RetirementOutcome.DEFERRED_IO_ERROR + try: + if self._is_current is not None and self._is_current(path): + return RetirementOutcome.DEFERRED_CONTENDED + try: + identity = identity_for_path(path) + except ( + PluginArtifactValidationError, + PluginArtifactUnavailableError, + OSError, + ): + # Cannot positively identify it; never delete on ambiguity. + return RetirementOutcome.LEGACY_EVIDENCE + created = self.enqueue_retirement(identity, now).created + remove_retiring_records((evidence.record_id,)) + finally: + writer.close_preserving() + return RetirementOutcome.RECLAIMED if created else RetirementOutcome.RECORD_REMOVED + def _log_reclaim( self, record: RetiringArtifactRecord, diff --git a/src/autoskillit/core/types/_type_plugin_source.py b/src/autoskillit/core/types/_type_plugin_source.py index 39c22d91f..8b403bf14 100644 --- a/src/autoskillit/core/types/_type_plugin_source.py +++ b/src/autoskillit/core/types/_type_plugin_source.py @@ -59,10 +59,23 @@ def consumes_artifact(self) -> bool: class PluginArtifactKind(StrEnum): - """Managed plugin artifact families sharing the retirement queue.""" + """Managed plugin artifact families sharing the retirement queue. + + ``PLUGIN_GENERATION`` routes the generation store + (``~/.autoskillit/plugin-generations/``) to its own retirement owner. + ``INSTALLED_PLUGIN`` remains bound to the legacy Claude-cache tree, which + ``reconcile_install_artifacts()`` still enqueues. The two roots are + disjoint, so one owner cannot serve both: a record routed to an owner whose + ``managed_root`` does not contain it is rejected on every sweep forever. + + This is a routing key for the retirement queue only. The on-disk manifest's + own ``artifact_kind`` stays ``INSTALLED_PLUGIN`` for both trees, since they + share one manifest format. + """ PROJECTION = "projection" INSTALLED_PLUGIN = "installed_plugin" + PLUGIN_GENERATION = "plugin_generation" class RetiringCacheState(StrEnum): diff --git a/src/autoskillit/core/types/_type_protocols_workspace.py b/src/autoskillit/core/types/_type_protocols_workspace.py index 04362305e..ae931cd3b 100644 --- a/src/autoskillit/core/types/_type_protocols_workspace.py +++ b/src/autoskillit/core/types/_type_protocols_workspace.py @@ -12,6 +12,7 @@ from ._type_enums import SkillExecutionRole, SkillInvalidityKind, SkillSource from ._type_exploration import RepositoryProfileId from ._type_plugin_source import ( + LegacyRetiringEvidence, PluginArtifactIdentity, PluginLaunchBinding, PluginLoadMode, @@ -87,6 +88,12 @@ def try_reclaim( now: datetime, ) -> RetirementOutcome: ... + def try_promote_legacy_evidence( + self, + evidence: LegacyRetiringEvidence, + now: datetime, + ) -> RetirementOutcome: ... + @runtime_checkable class PluginRetirementCoordinator(Protocol): diff --git a/src/autoskillit/execution/backends/_codex_hooks.py b/src/autoskillit/execution/backends/_codex_hooks.py index 5691ceb8a..532f6b0ac 100644 --- a/src/autoskillit/execution/backends/_codex_hooks.py +++ b/src/autoskillit/execution/backends/_codex_hooks.py @@ -87,6 +87,17 @@ def _resolve_codex_hooks_dir(plugin_dir: Path | None = None) -> Path: short-lived resolve→validate of the current generation selector is performed through the same generation-store authority as launch binding. + The bindingless path prefers the *version-independent* selector. Codex + bakes this absolute path into ``~/.codex/config.toml`` and re-reads it at + every launch without re-resolving, and a per-session snapshot of that + config may outlive the version it was written against. A path pinned to one + version's incarnation therefore dangles as soon as that generation is + retired, taking every safety guard with it. The plugin-level selector is + re-pointed on each publish, so the same absolute path stays valid across + version bumps. ``trusted_hash`` is unaffected: it hashes the dispatcher's + bytes through the symlink, and ``_dispatch.py`` is byte-identical across + versions by design. + If neither the generation store nor the legacy installed cache supplies a dispatcher, the dev-checkout ``HOOKS_DIR`` is used as the terminal fallback. """ @@ -98,7 +109,16 @@ def _resolve_codex_hooks_dir(plugin_dir: Path | None = None) -> Path: # Bindingless path: resolve from generation store with short-lived lease from autoskillit import __version__ - from autoskillit.core import resolve_current_generation + from autoskillit.core import ( + generation_plugin_selector_path, + resolve_current_generation, + resolve_current_generation_for_plugin, + ) + + if resolve_current_generation_for_plugin(Path.home(), "autoskillit") is not None: + candidate = generation_plugin_selector_path(Path.home(), "autoskillit") / "hooks" + if (candidate / "_dispatch.py").is_file(): + return candidate generation_dir = resolve_current_generation(Path.home(), "autoskillit", __version__) if generation_dir is not None: diff --git a/src/autoskillit/workspace/__init__.py b/src/autoskillit/workspace/__init__.py index c04154764..188fc5f87 100644 --- a/src/autoskillit/workspace/__init__.py +++ b/src/autoskillit/workspace/__init__.py @@ -45,6 +45,8 @@ validate_staged_plugin_hooks, ) from autoskillit.workspace._projected_artifact._generation_publication import ( + GenerationArtifactRetirementOwner, + prune_stale_generations, publish_generation, ) from autoskillit.workspace._update_obligation import ( @@ -204,6 +206,8 @@ "SkillProjectionContext", "iter_public_plugin_asset_files", "marketplace_plugin_root", + "GenerationArtifactRetirementOwner", + "prune_stale_generations", "prune_stale_projections", "public_plugin_asset_digest", "reconcile_install_artifacts", diff --git a/src/autoskillit/workspace/_projected_artifact/_generation_publication.py b/src/autoskillit/workspace/_projected_artifact/_generation_publication.py index 859212d9a..00b3f1b08 100644 --- a/src/autoskillit/workspace/_projected_artifact/_generation_publication.py +++ b/src/autoskillit/workspace/_projected_artifact/_generation_publication.py @@ -19,12 +19,20 @@ from autoskillit.core import ( ArtifactLease, + ArtifactLeaseContention, + LegacyRetiringEvidence, PluginArtifactIdentity, PluginArtifactKind, + PluginArtifactRetirementEngine, PluginArtifactValidationError, + RetirementOutcome, + RetiringAppendResult, + RetiringArtifactRecord, directory_tree_digest, generation_artifact_root, + generation_plugin_selector_path, generation_selector_path, + generation_store_root, generation_version_root, get_logger, installed_plugin_artifact_lease_path, @@ -33,6 +41,7 @@ new_plugin_artifact_incarnation_id, read_installed_plugin_artifact_identity, resolve_current_generation, + resolve_current_generation_for_plugin, ) from autoskillit.workspace._installed_artifact import ( write_installed_plugin_artifact_manifest_locked, @@ -42,6 +51,12 @@ _STAGING_ORPHAN_GRACE = timedelta(hours=1) +# Cross-version staleness needs a wider window than same-version churn: the +# retirement sweep runs once per MCP server startup, not on a recurring timer +# (server/_lifespan.py fires it once), so the grace must comfortably outlast the +# gap between server restarts on a lightly-used machine. +_GENERATION_GRACE = timedelta(hours=24) + def _sweep_orphaned_staging(version_root: Path) -> None: """Remove staging directories abandoned by a crashed ``publish_generation`` call. @@ -238,9 +253,8 @@ def publish_generation( incarnation=incarnation_id, ) - # Enqueue prior generation for retirement (Phase 4.6) - if prior_target is not None: - _enqueue_prior_generation(prior_target, version_root, home, plugin_ref, version) + _select_plugin_generation(home, plugin_ref, generation_root) + prune_stale_generations(home, plugin_ref) return PluginArtifactIdentity( semantic_key=semantic_key, @@ -252,49 +266,194 @@ def publish_generation( ) -def _enqueue_prior_generation( - prior_target: Path, - version_root: Path, - home: Path, - plugin_ref: str, - version: str, -) -> None: - """Enqueue a superseded generation into the retirement engine. +def _select_plugin_generation(home: Path, plugin_ref: str, generation_root: Path) -> None: + """Point the version-independent selector at the newly published generation. - Best-effort: failure to enqueue is logged but does not fail the - publication — an orphan sweep will catch it later. + Best-effort, mirroring the retirement enqueue below it: the per-version flip + is already durable and must not be rolled back if this one fails. A + persistent failure fails safe — the stale target stays protected by + ``_is_selected_generation`` and is over-retained rather than reclaimed. """ - from autoskillit.core import PluginArtifactRetirementEngine - + selector = generation_plugin_selector_path(home, plugin_ref) try: - prior_identity = read_installed_plugin_artifact_identity( - prior_target, - manifest_path=installed_plugin_artifact_manifest_path(prior_target), - ) - except (PluginArtifactValidationError, OSError) as exc: + selector.parent.mkdir(parents=True, exist_ok=True) + _replace_symlink(selector, generation_root) + except OSError as exc: logger.warning( - "generation_retirement_enqueue_skipped: could not read prior generation identity: %s", + "generation_plugin_selector_flip_failed: %s: %s", + selector, exc, ) - return - engine = PluginArtifactRetirementEngine( - managed_root=version_root, - artifact_kind=PluginArtifactKind.INSTALLED_PLUGIN, - manifest_path=installed_plugin_artifact_manifest_path, - lease_path=installed_plugin_artifact_lease_path, - current_identity=lambda record: read_installed_plugin_artifact_identity( + + +def _is_selected_generation(home: Path, plugin_ref: str, path: Path) -> bool: + """Return whether *path* is still selected and therefore must not be retired. + + Every superseded *version* keeps its own per-version ``current`` symlink + pointing at its own incarnation forever — nothing rewrites it when a newer + version is published. Treating that as protection is precisely why no + generation was ever reclaimable: each version vouched for itself. + + So once the plugin-level selector exists it is authoritative. It names the + live generation, and only that generation's version keeps its per-version + selector honored (a consumer that resolved through the per-version path + just before the plugin-level flip may still be using it). + + Before any plugin-level selector exists — a first publish, or a persistent + flip failure — fall back to per-version protection, which over-retains + rather than deleting something still in use. + """ + plugin_selected = resolve_current_generation_for_plugin(home, plugin_ref) + if plugin_selected is None: + return path == resolve_current_generation(home, plugin_ref, path.parent.name) + if path == plugin_selected: + return True + return path == resolve_current_generation(home, plugin_ref, plugin_selected.parent.name) + + +class GenerationArtifactRetirementOwner: + """Exact-identity retirement owner for the whole generation store. + + Scoped to ``generation_store_root`` — every version, not one — because the + retirement coordinator dispatches by artifact kind to a single owner. An + owner rooted at one version directory cannot contain records from any other, + and ``try_reclaim`` rejects an uncontained record on every sweep forever + without ever removing it. + """ + + def __init__(self, managed_root: Path, *, home: Path, plugin_ref: str) -> None: + self._home = Path(home) + self._plugin_ref = plugin_ref + self._retirement = PluginArtifactRetirementEngine( + managed_root=managed_root, + artifact_kind=PluginArtifactKind.PLUGIN_GENERATION, + manifest_path=self.manifest_path, + lease_path=self.lease_path, + current_identity=self._current_identity, + logger=logger, + is_current=lambda path: _is_selected_generation(self._home, self._plugin_ref, path), + ) + + @property + def managed_root(self) -> Path: + return self._retirement.managed_root + + @staticmethod + def manifest_path(managed_path: Path) -> Path: + return installed_plugin_artifact_manifest_path(managed_path) + + @staticmethod + def lease_path(managed_path: Path) -> Path: + return installed_plugin_artifact_lease_path(managed_path) + + def enqueue_retirement( + self, + identity: PluginArtifactIdentity, + not_before: datetime, + ) -> RetiringAppendResult: + return self._retirement.enqueue_retirement(identity, not_before) + + def cancel_obsolete_retirements(self, identity: PluginArtifactIdentity) -> tuple[str, ...]: + return self._retirement.cancel_obsolete_retirements(identity) + + def identity_for_path(self, managed_path: Path) -> PluginArtifactIdentity: + """Validate and return the exact current identity at a managed path.""" + managed_path = Path(managed_path) + if not self._retirement.contains(managed_path): + raise PluginArtifactValidationError( + f"generation is outside managed root: {managed_path}" + ) + return read_installed_plugin_artifact_identity( + managed_path, + manifest_path=self.manifest_path(managed_path), + ) + + def _current_identity(self, record: RetiringArtifactRecord) -> PluginArtifactIdentity: + return read_installed_plugin_artifact_identity( record.managed_path, expected_semantic_key=record.semantic_key, - manifest_path=installed_plugin_artifact_manifest_path(record.managed_path), - ), - logger=logger, - is_current=lambda path: path == resolve_current_generation(home, plugin_ref, version), - ) - deadline = datetime.now(UTC) + timedelta(hours=6) - try: - engine.enqueue_retirement(prior_identity, deadline) - except Exception as exc: - logger.warning( - "generation_retirement_enqueue_failed: %s", - exc, + manifest_path=self.manifest_path(record.managed_path), + ) + + def try_reclaim(self, record: RetiringArtifactRecord, now: datetime) -> RetirementOutcome: + return self._retirement.try_reclaim(record, now) + + def try_promote_legacy_evidence( + self, + evidence: LegacyRetiringEvidence, + now: datetime, + ) -> RetirementOutcome: + return self._retirement.try_promote_legacy_evidence( + evidence, + now, + identity_for_path=self.identity_for_path, ) + + +def prune_stale_generations(home: Path, plugin_ref: str) -> int: + """Queue every superseded generation across all versions for retirement. + + Enqueue-only: nothing is deleted here. Actual removal flows through + ``try_reclaim``, which re-checks the lease and exact identity under its own + grace window. + + Must be called under ``_InstallLock`` by the caller, like + ``publish_generation`` itself — the lock is a non-reentrant ``flock``, so + re-acquiring it from inside a publish would deadlock against the caller. + + Called at publish time only. Enqueueing recomputes a full content-tree + digest per candidate, so wiring this into the session-launch path would + re-hash the entire backlog on every launch until each entry is reclaimed. + Publication is the only event that can create staleness, so a machine that + stops updating never grows a new backlog either. + """ + store_root = generation_store_root(home, plugin_ref) + if not store_root.is_dir(): + return 0 + owner = GenerationArtifactRetirementOwner(store_root, home=home, plugin_ref=plugin_ref) + candidates: list[Path] = [] + for version_dir in sorted(store_root.iterdir(), key=lambda item: item.name): + if version_dir.name.startswith(".") or version_dir.is_symlink(): + continue + if not version_dir.is_dir(): + continue + for incarnation in sorted(version_dir.iterdir(), key=lambda item: item.name): + if incarnation.name.startswith(".") or incarnation.is_symlink(): + continue + if not incarnation.is_dir(): + continue + if _is_selected_generation(home, plugin_ref, incarnation): + continue + candidates.append(incarnation) + + created = 0 + not_before = datetime.now(UTC) + _GENERATION_GRACE + for candidate in candidates: + try: + writer = ArtifactLease.acquire_exclusive( + owner.lease_path(candidate), + blocking=False, + ) + except ArtifactLeaseContention: + continue + except (OSError, RuntimeError) as exc: + logger.warning( + "generation_prune_lease_failed: %s: %s", + candidate, + exc, + ) + continue + try: + try: + identity = owner.identity_for_path(candidate) + except (PluginArtifactValidationError, OSError) as exc: + logger.warning( + "generation_prune_validation_failed: %s: %s", + candidate, + exc, + ) + continue + created += int(owner.enqueue_retirement(identity, not_before).created) + finally: + writer.close_preserving() + return created diff --git a/src/autoskillit/workspace/_projection_cache.py b/src/autoskillit/workspace/_projection_cache.py index bc2a5cfd2..a9092b2b5 100644 --- a/src/autoskillit/workspace/_projection_cache.py +++ b/src/autoskillit/workspace/_projection_cache.py @@ -21,6 +21,7 @@ from autoskillit.core import ( ArtifactLease, ArtifactLeaseContention, + LegacyRetiringEvidence, PluginArtifactIdentity, PluginArtifactKind, PluginArtifactRetirementEngine, @@ -412,6 +413,17 @@ def cancel_obsolete_retirements( ) -> tuple[str, ...]: return self._retirement.cancel_obsolete_retirements(identity) + def try_promote_legacy_evidence( + self, + evidence: LegacyRetiringEvidence, + now: datetime, + ) -> RetirementOutcome: + return self._retirement.try_promote_legacy_evidence( + evidence, + now, + identity_for_path=self.identity_for_path, + ) + def identity_for_path(self, managed_path: Path) -> PluginArtifactIdentity: """Validate and return the exact current identity at a managed path.""" managed_path = Path(managed_path) diff --git a/tests/arch/test_subpackage_isolation.py b/tests/arch/test_subpackage_isolation.py index 19d93273b..dee9f4dbb 100644 --- a/tests/arch/test_subpackage_isolation.py +++ b/tests/arch/test_subpackage_isolation.py @@ -1086,6 +1086,17 @@ def test_data_directories_are_not_python_packages() -> None: # original single-responsibility scope (REQ-CNST-010-NOTE-1). _LINE_LIMIT_EXEMPTIONS: dict[str, tuple[int, str]] = { + "core/_plugin_cache.py": ( + 1100, + "REQ-CNST-010-E26: #4689 added try_promote_legacy_evidence beside try_reclaim. " + "Both mutate the retiring cache under the install lock and must stay adjacent to " + "the append/remove/read primitives they call, for the same reason " + "_projected_artifact/AGENTS.md keeps publication beside lease handoff: splitting " + "them puts lock ordering across a module boundary, which is how destructive " + "repair bypasses the lifecycle lock. tests/infra/test_plugin_source_ratchets.py " + "also pins this module's raw-mutation call sites by (file, function, expression), " + "so the reclaim path's location is a checked invariant, not an accident.", + ), "execution/evidence_reader.py": ( 1500, "REQ-CNST-010-E25: #4585 keeps sterile auth, projection, probes, managed process " diff --git a/tests/contracts/test_generation_retirement.py b/tests/contracts/test_generation_retirement.py new file mode 100644 index 000000000..68159819e --- /dev/null +++ b/tests/contracts/test_generation_retirement.py @@ -0,0 +1,255 @@ +"""The generation store must actually be reclaimable, and never eat its own infrastructure. + +Three defects motivated these tests, all shipped together and all invisible to the +prior suite: + +1. ``default_plugin_retirement_coordinator()`` routed every ``INSTALLED_PLUGIN`` + record to an owner rooted at the *legacy* Claude plugin cache, while + ``publish_generation()`` writes records under ``~/.autoskillit/plugin-generations``. + ``try_reclaim`` rejects an uncontained record **without removing it**, so no + generation was ever reclaimable — the queue only grew. +2. Retirement was enqueued per-version, so a superseded *version* was never + queued at all. +3. Migrated ``legacy_evidence`` recorded ``plugin-projections/.artifact-leases`` — + the directory holding every live session's lease locks — as a ``projection``. + Anything that reclaims legacy evidence by trusting that stored classification + deletes the lease infrastructure out from under running sessions. + +Every test here fails against the pre-fix code. +""" + +from __future__ import annotations + +from datetime import UTC, datetime, timedelta +from pathlib import Path + +import pytest + +from autoskillit.core import ( + LegacyRetiringEvidence, + PluginArtifactIdentity, + PluginArtifactKind, + RetirementOutcome, + _InstallLock, + generation_plugin_selector_path, + is_reclaimable_artifact_path, + read_retiring_cache, + resolve_current_generation_for_plugin, +) + +pytestmark = [pytest.mark.layer("contracts"), pytest.mark.medium] + +_PLUGIN_REF = "autoskillit" + + +def _publish(home: Path, source_root: Path, version: str) -> PluginArtifactIdentity: + from autoskillit.workspace import publish_generation + + with _InstallLock(): + return publish_generation( + home=home, + plugin_ref=_PLUGIN_REF, + version=version, + semantic_key=f"autoskillit@autoskillit-local:{version}", + source_root=source_root, + ) + + +@pytest.fixture +def home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + monkeypatch.setattr(Path, "home", lambda: tmp_path) + return tmp_path + + +@pytest.fixture +def source_root(tmp_path: Path) -> Path: + root = tmp_path / "source" + (root / "hooks").mkdir(parents=True) + (root / "hooks" / "_dispatch.py").write_text("# dispatcher\n", encoding="utf-8") + return root + + +# --------------------------------------------------------------------------- +# The infrastructure landmine +# --------------------------------------------------------------------------- + + +def test_lease_directory_is_never_a_reclaimable_artifact() -> None: + """``.artifact-leases`` holds every live session's lock; it is not an artifact.""" + managed_root = Path("/managed/plugin-projections") + + assert not is_reclaimable_artifact_path(managed_root / ".artifact-leases", managed_root) + assert not is_reclaimable_artifact_path(managed_root / ".anything-hidden", managed_root) + assert is_reclaimable_artifact_path(managed_root / "0a1064ba3c624c945ca0e2df", managed_root) + + +def test_nested_paths_are_not_artifacts() -> None: + """An artifact is exactly one level deep; deeper paths are its components.""" + managed_root = Path("/managed/plugin-projections") + + assert not is_reclaimable_artifact_path(managed_root / "abc" / "hooks", managed_root) + assert not is_reclaimable_artifact_path(managed_root, managed_root) + + +def test_classification_refuses_infrastructure_paths(tmp_path: Path) -> None: + """Migration must not record the lease directory as a retirable projection.""" + from autoskillit.core._plugin_cache import _classify_legacy_path + + projections = tmp_path / "plugin-projections" + leases = projections / ".artifact-leases" + leases.mkdir(parents=True) + roots = {PluginArtifactKind.PROJECTION: projections} + + kind, reason = _classify_legacy_path(str(leases), roots) + + assert kind is None + assert reason is not None and "infrastructure" in reason + + +def test_promotion_refuses_infrastructure_even_when_evidence_says_projection( + tmp_path: Path, +) -> None: + """Already-persisted evidence carries the bad classification; re-derive it. + + The stored ``recognized_kind`` is exactly what the old migration got wrong, + so the sweep must never treat it as authority. + """ + from autoskillit.workspace import ProjectedPluginRetirementOwner + + projections = tmp_path / "plugin-projections" + leases = projections / ".artifact-leases" + leases.mkdir(parents=True) + (leases / "somehash.lock").write_text("", encoding="utf-8") + + owner = ProjectedPluginRetirementOwner(projections) + evidence = LegacyRetiringEvidence( + record_id="deadbeef", + version="projection:.artifact-leases", + path=str(leases), + retired_at="2026-07-29T03:18:17.568199+00:00", + recognized_kind=PluginArtifactKind.PROJECTION, + rejection_reason=None, + ) + + outcome = owner.try_promote_legacy_evidence(evidence, datetime.now(UTC)) + + assert outcome is RetirementOutcome.LEGACY_EVIDENCE + assert leases.is_dir(), "the live lease directory must survive" + assert (leases / "somehash.lock").is_file() + + +def test_promotion_drops_bookkeeping_for_already_gone_paths(tmp_path: Path) -> None: + """A duplicate row for an already-deleted artifact resolves with no I/O.""" + from autoskillit.core import append_retiring_record # noqa: F401 (cache bootstrap) + from autoskillit.workspace import ProjectedPluginRetirementOwner + + projections = tmp_path / "plugin-projections" + projections.mkdir(parents=True) + owner = ProjectedPluginRetirementOwner(projections) + evidence = LegacyRetiringEvidence( + record_id="cafebabe", + version="projection:gone", + path=str(projections / "gone"), + retired_at="2026-07-29T03:18:17.568199+00:00", + recognized_kind=PluginArtifactKind.PROJECTION, + rejection_reason=None, + ) + + outcome = owner.try_promote_legacy_evidence(evidence, datetime.now(UTC)) + + assert outcome is RetirementOutcome.RECORD_REMOVED + + +# --------------------------------------------------------------------------- +# Cross-version retirement + correct routing +# --------------------------------------------------------------------------- + + +def test_superseded_version_is_enqueued_on_publish(home: Path, source_root: Path) -> None: + """A new version must queue the prior version, not just prior incarnations.""" + first = _publish(home, source_root, "1.0.0") + (source_root / "hooks" / "_dispatch.py").write_text("# v2\n", encoding="utf-8") + _publish(home, source_root, "2.0.0") + + queued = {record.managed_path for record in read_retiring_cache().records} + + assert first.managed_path in queued, "the superseded 1.0.0 generation must be queued" + + +def test_queued_generation_is_reclaimable_by_the_default_coordinator( + home: Path, + source_root: Path, +) -> None: + """The regression: records were routed to an owner that could never contain them.""" + from autoskillit.cli._plugin_artifact import default_plugin_retirement_coordinator + + first = _publish(home, source_root, "1.0.0") + (source_root / "hooks" / "_dispatch.py").write_text("# v2\n", encoding="utf-8") + _publish(home, source_root, "2.0.0") + + coordinator = default_plugin_retirement_coordinator() + outcomes = coordinator.sweep_due(datetime.now(UTC) + timedelta(days=2)) + + assert RetirementOutcome.RECLAIMED in outcomes + assert not first.managed_path.exists(), "the superseded generation must be removed" + + +def test_selected_generations_are_never_reclaimed(home: Path, source_root: Path) -> None: + """Both selectors protect: per-version current and the plugin-level current.""" + from autoskillit.cli._plugin_artifact import default_plugin_retirement_coordinator + + _publish(home, source_root, "1.0.0") + (source_root / "hooks" / "_dispatch.py").write_text("# v2\n", encoding="utf-8") + current = _publish(home, source_root, "2.0.0") + + coordinator = default_plugin_retirement_coordinator() + coordinator.sweep_due(datetime.now(UTC) + timedelta(days=2)) + + assert current.managed_path.is_dir() + assert resolve_current_generation_for_plugin(home, _PLUGIN_REF) == current.managed_path + + +def test_every_artifact_kind_has_a_registered_owner(home: Path) -> None: + """A new storage location without an owner silently repeats the routing bug.""" + from autoskillit.cli._plugin_artifact import default_plugin_retirement_coordinator + + coordinator = default_plugin_retirement_coordinator() + + assert frozenset(coordinator._owners) == frozenset(PluginArtifactKind) + + +# --------------------------------------------------------------------------- +# The version-independent selector Codex pins +# --------------------------------------------------------------------------- + + +def test_plugin_selector_survives_a_version_bump(home: Path, source_root: Path) -> None: + """Codex bakes this absolute path into config; it must not dangle on bump.""" + _publish(home, source_root, "1.0.0") + selector = generation_plugin_selector_path(home, _PLUGIN_REF) + dispatcher = selector / "hooks" / "_dispatch.py" + + assert dispatcher.is_file() + + (source_root / "hooks" / "_dispatch.py").write_text("# v2\n", encoding="utf-8") + second = _publish(home, source_root, "2.0.0") + + assert dispatcher.is_file(), "the pinned path must still resolve after a bump" + assert selector.resolve() == second.managed_path + + +def test_codex_hooks_resolve_through_the_plugin_selector( + home: Path, + source_root: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The bindingless resolver must prefer the version-independent path.""" + from autoskillit.execution.backends._codex_hooks import _resolve_codex_hooks_dir + + _publish(home, source_root, "1.0.0") + monkeypatch.setattr("autoskillit.__version__", "9.9.9", raising=False) + + resolved = _resolve_codex_hooks_dir() + + assert resolved == generation_plugin_selector_path(home, _PLUGIN_REF) / "hooks" + assert (resolved / "_dispatch.py").is_file() From a05d48a94be1978acd4e99b519882270bbee5826 Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 18 Aug 2026 12:47:34 -0700 Subject: [PATCH 2/7] fix(review): harmonize identity_for_path containment check across all three retirement owners InstalledPluginArtifactRetirementOwner.identity_for_path was the only one of the three owner implementations (Installed/Projected/Generation) that skipped the containment check before validating identity, so a caller holding an out-of-root path got a different exception class here than from the other two owners. Add the same self._contains() guard + PluginArtifactValidationError already used by ProjectedPluginRetirementOwner and GenerationArtifactRetirementOwner. Addresses review findings at cli/_plugin_artifact.py:439 and workspace/_projected_artifact/_generation_publication.py:359 (same underlying asymmetry, two vantage points). --- src/autoskillit/cli/_plugin_artifact.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/autoskillit/cli/_plugin_artifact.py b/src/autoskillit/cli/_plugin_artifact.py index 6bfed4d83..001534e04 100644 --- a/src/autoskillit/cli/_plugin_artifact.py +++ b/src/autoskillit/cli/_plugin_artifact.py @@ -438,6 +438,11 @@ def try_promote_legacy_evidence( def identity_for_path(self, managed_path: Path) -> PluginArtifactIdentity: """Validate and return the exact current identity at a managed path.""" + managed_path = Path(managed_path) + if not self._contains(managed_path): + raise PluginArtifactValidationError( + f"installed plugin is outside managed root: {managed_path}" + ) return _read_and_validate_identity( managed_path, expected_semantic_key=installed_plugin_semantic_key( From fe6af9d56ce54dadfb3d5c204178e62bd2cf4a7a Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 18 Aug 2026 12:49:09 -0700 Subject: [PATCH 3/7] fix(review): catch TypeError from Path(evidence.path) in try_promote_legacy_evidence evidence.path being None or otherwise non-string-coercible raises TypeError from Path(), which the existing except (OSError, ValueError) does not catch, aborting the whole sweep pass. Add TypeError to the caught tuple so a malformed legacy-evidence record degrades to LEGACY_EVIDENCE instead. Addresses review finding at core/_plugin_cache.py (try_promote_legacy_evidence). --- src/autoskillit/core/_plugin_cache.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/autoskillit/core/_plugin_cache.py b/src/autoskillit/core/_plugin_cache.py index 25b831a6e..8fad57a25 100644 --- a/src/autoskillit/core/_plugin_cache.py +++ b/src/autoskillit/core/_plugin_cache.py @@ -747,21 +747,28 @@ def try_promote_legacy_evidence( return RetirementOutcome.LEGACY_EVIDENCE try: path = destination_location(Path(evidence.path)) - except (OSError, ValueError): + except (OSError, TypeError, ValueError): return RetirementOutcome.LEGACY_EVIDENCE if not is_reclaimable_artifact_path(path, self.managed_root): return RetirementOutcome.LEGACY_EVIDENCE if not self.contains(path): return RetirementOutcome.LEGACY_EVIDENCE - if not path.exists() and not path.is_symlink(): - # Nothing left to protect; drop the bookkeeping without any I/O. + if not path.exists(): + # Nothing left to protect (also covers a broken symlink: exists() + # follows the link and is False when the target is gone). remove_retiring_records((evidence.record_id,)) return RetirementOutcome.RECORD_REMOVED try: writer = ArtifactLease.acquire_exclusive(self._lease_path(path), blocking=False) except ArtifactLeaseContention: return RetirementOutcome.DEFERRED_CONTENDED - except (OSError, RuntimeError): + except (OSError, RuntimeError) as exc: + self._logger.warning( + "plugin_artifact_legacy_promotion_lease_failed", + artifact_kind=self.artifact_kind.value, + path=str(path), + error=str(exc), + ) return RetirementOutcome.DEFERRED_IO_ERROR try: if self._is_current is not None and self._is_current(path): From 2a2f4e848578940bcacac476a6d4b865bd57cfeb Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 18 Aug 2026 12:49:49 -0700 Subject: [PATCH 4/7] fix(review): use _AUTOSKILLIT_PLUGIN_KEY constant instead of literal 'autoskillit' in _resolve_codex_hooks_dir The bindingless resolution path hardcoded the string literal 'autoskillit' at four call sites in the same function that already imports and uses _AUTOSKILLIT_PLUGIN_KEY for the sibling semantic-key computation a few lines below. A future plugin-key change would silently desync this resolver from the constant it already depends on elsewhere in the same function. Addresses review finding at execution/backends/_codex_hooks.py:118. --- src/autoskillit/execution/backends/_codex_hooks.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/autoskillit/execution/backends/_codex_hooks.py b/src/autoskillit/execution/backends/_codex_hooks.py index 532f6b0ac..8c1a53201 100644 --- a/src/autoskillit/execution/backends/_codex_hooks.py +++ b/src/autoskillit/execution/backends/_codex_hooks.py @@ -115,19 +115,19 @@ def _resolve_codex_hooks_dir(plugin_dir: Path | None = None) -> Path: resolve_current_generation_for_plugin, ) - if resolve_current_generation_for_plugin(Path.home(), "autoskillit") is not None: - candidate = generation_plugin_selector_path(Path.home(), "autoskillit") / "hooks" + if resolve_current_generation_for_plugin(Path.home(), _AUTOSKILLIT_PLUGIN_KEY) is not None: + candidate = generation_plugin_selector_path(Path.home(), _AUTOSKILLIT_PLUGIN_KEY) / "hooks" if (candidate / "_dispatch.py").is_file(): return candidate - generation_dir = resolve_current_generation(Path.home(), "autoskillit", __version__) + generation_dir = resolve_current_generation(Path.home(), _AUTOSKILLIT_PLUGIN_KEY, __version__) if generation_dir is not None: candidate = generation_dir / "hooks" if (candidate / "_dispatch.py").is_file(): return candidate # Fall back to legacy installed cache - cache_root = installed_plugin_artifact_root(Path.home(), "autoskillit", __version__) + cache_root = installed_plugin_artifact_root(Path.home(), _AUTOSKILLIT_PLUGIN_KEY, __version__) try: identity = read_installed_plugin_artifact_identity( cache_root, From 847059bfb61fca8fbeff09eaefea73f3389d19da Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 18 Aug 2026 12:50:24 -0700 Subject: [PATCH 5/7] refactor(review): trim bug-history narrative from two docstrings _is_selected_generation and prune_stale_generations docstrings each carried a paragraph of historical/performance-rationale narrative duplicating the PR commit message. Kept the load-bearing behavioral contract (dual-branch fallback logic; the session-launch-vs-publish-time warning) and cut the narrative exposition. Addresses info findings at workspace/_projected_artifact/_generation_publication.py:289 and :393. --- .../_generation_publication.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/autoskillit/workspace/_projected_artifact/_generation_publication.py b/src/autoskillit/workspace/_projected_artifact/_generation_publication.py index 00b3f1b08..61722f5f6 100644 --- a/src/autoskillit/workspace/_projected_artifact/_generation_publication.py +++ b/src/autoskillit/workspace/_projected_artifact/_generation_publication.py @@ -289,12 +289,7 @@ def _select_plugin_generation(home: Path, plugin_ref: str, generation_root: Path def _is_selected_generation(home: Path, plugin_ref: str, path: Path) -> bool: """Return whether *path* is still selected and therefore must not be retired. - Every superseded *version* keeps its own per-version ``current`` symlink - pointing at its own incarnation forever — nothing rewrites it when a newer - version is published. Treating that as protection is precisely why no - generation was ever reclaimable: each version vouched for itself. - - So once the plugin-level selector exists it is authoritative. It names the + Once the plugin-level selector exists it is authoritative. It names the live generation, and only that generation's version keeps its per-version selector honored (a consumer that resolved through the per-version path just before the plugin-level flip may still be using it). @@ -401,11 +396,10 @@ def prune_stale_generations(home: Path, plugin_ref: str) -> int: ``publish_generation`` itself — the lock is a non-reentrant ``flock``, so re-acquiring it from inside a publish would deadlock against the caller. - Called at publish time only. Enqueueing recomputes a full content-tree - digest per candidate, so wiring this into the session-launch path would - re-hash the entire backlog on every launch until each entry is reclaimed. - Publication is the only event that can create staleness, so a machine that - stops updating never grows a new backlog either. + Called at publish time only — wiring this into the session-launch path + would re-hash the entire backlog (a full content-tree digest per + candidate) on every launch, since publication is the only event that can + create staleness. """ store_root = generation_store_root(home, plugin_ref) if not store_root.is_dir(): From 73a667a43099655b931c91522f7971a997659500 Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 18 Aug 2026 12:50:59 -0700 Subject: [PATCH 6/7] refactor(review): trim bug-history narrative from two docstrings is_reclaimable_artifact_path and try_promote_legacy_evidence docstrings each carried a sentence of historical-bug narrative (a past prune_stale_projections defect; a past .artifact-leases classification bug) better suited to a commit message. Kept the load-bearing safety contract in each and cut the narrative. Addresses info findings at core/_plugin_cache.py:297 and :726. --- src/autoskillit/core/_plugin_cache.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/autoskillit/core/_plugin_cache.py b/src/autoskillit/core/_plugin_cache.py index 8fad57a25..5b6d8ebf9 100644 --- a/src/autoskillit/core/_plugin_cache.py +++ b/src/autoskillit/core/_plugin_cache.py @@ -300,10 +300,7 @@ def is_reclaimable_artifact_path(path: Path, managed_root: Path) -> bool: Only a direct, non-hidden child of *managed_root* qualifies. Dot-prefixed entries are managed infrastructure, never artifacts — most importantly ``plugin-projections/.artifact-leases``, the directory holding the lock - files every live session's inherited reader lease is held on. An earlier - ``prune_stale_projections`` enumerated that root without a dot-prefix filter - and queued the lease directory alongside real projections, so persisted - evidence recording it as a ``projection`` already exists on disk. Reclaiming + files every live session's inherited reader lease is held on. Reclaiming it would delete the lease infrastructure out from under every running session. @@ -739,9 +736,8 @@ def try_promote_legacy_evidence( record. Nothing is deleted here. Eligibility is re-derived from scratch rather than trusting the stored - ``recognized_kind``: that field is exactly what a past classification - bug got wrong for ``.artifact-leases``, and evidence already persisted - with a wrong kind must not become authority now. + ``recognized_kind`` — evidence already persisted with a wrong kind + must not become authority now. """ if evidence.recognized_kind is not self.artifact_kind: return RetirementOutcome.LEGACY_EVIDENCE From 95ff8d81b22d0eb8d7cda427031ebc91844d3553 Mon Sep 17 00:00:00 2001 From: Trecek Date: Tue, 18 Aug 2026 12:51:22 -0700 Subject: [PATCH 7/7] test(review): close two vacuous-pass gaps in generation retirement contract tests test_superseded_version_is_enqueued_on_publish only asserted the superseded generation was queued; add the complementary assertion that the currently selected generation is NOT queued. test_selected_generations_are_never_reclaimed discarded sweep_due()'s outcome tuple, so it would pass vacuously if the sweep silently did nothing (directory-existence and selector-resolution checks would still hold). Assert RetirementOutcome.RECLAIMED is present to prove 1.0.0 was actually processed while 2.0.0 (current) survives. Addresses info findings at tests/contracts/test_generation_retirement.py:168 and :197. --- tests/contracts/test_generation_retirement.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/contracts/test_generation_retirement.py b/tests/contracts/test_generation_retirement.py index 68159819e..8c35393a0 100644 --- a/tests/contracts/test_generation_retirement.py +++ b/tests/contracts/test_generation_retirement.py @@ -169,11 +169,14 @@ def test_superseded_version_is_enqueued_on_publish(home: Path, source_root: Path """A new version must queue the prior version, not just prior incarnations.""" first = _publish(home, source_root, "1.0.0") (source_root / "hooks" / "_dispatch.py").write_text("# v2\n", encoding="utf-8") - _publish(home, source_root, "2.0.0") + second = _publish(home, source_root, "2.0.0") queued = {record.managed_path for record in read_retiring_cache().records} assert first.managed_path in queued, "the superseded 1.0.0 generation must be queued" + assert second.managed_path not in queued, ( + "the currently selected generation must not be queued" + ) def test_queued_generation_is_reclaimable_by_the_default_coordinator( @@ -203,8 +206,11 @@ def test_selected_generations_are_never_reclaimed(home: Path, source_root: Path) current = _publish(home, source_root, "2.0.0") coordinator = default_plugin_retirement_coordinator() - coordinator.sweep_due(datetime.now(UTC) + timedelta(days=2)) + outcomes = coordinator.sweep_due(datetime.now(UTC) + timedelta(days=2)) + assert RetirementOutcome.RECLAIMED in outcomes, ( + "the superseded 1.0.0 generation must be reclaimed" + ) assert current.managed_path.is_dir() assert resolve_current_generation_for_plugin(home, _PLUGIN_REF) == current.managed_path