Rectify: Contracted Update, Install, and Plugin Publication Transaction

[Trecek]

Summary

Issue #4409 is a split-transaction defect, not a Cook artifact-selection defect. An update can advance the AutoSkillit package, decline the required Claude plugin refresh, lose that semantic decline at the CLI boundary, verify only package metadata, and then report success and restart. Claude later selects installed-plugin mode and correctly fails closed because the exact current-version artifact does not exist. Codex continues through generated-home mode and masks the skew.

Related issue: #4409 — Codex-scoped update silently skips Claude plugin refresh and breaks Cook

Closes #4409

Implementation Plan

Plan file: /home/talon/projects/generic_automation_mcp/.autoskillit/temp/rectify/rectify_update_install_publication_transaction_2026-07-29_192008.md

🤖 Generated with Claude Code via AutoSkillit

[Trecek]

AutoSkillit PR Review — Verdict: changes_requested

[Trecek]

AutoSkillit review: blocking findings detected. GitHub does not allow the PR author to request changes on their own PR; see the 13 inline comments for required fixes.

[Trecek]

AutoSkillit PR Review — Verdict: approved_with_comments

Five warning-level findings posted inline below. Five info-level observations (no-op callback, dead params, asymmetric result builder, self-alias re-export, dead nested install_result field) are summarised here:

• L224 [info/cohesion] src/autoskillit/cli/update/_update_checks.py: [info] cohesion: _run_update_sequence keeps info and current in its signature only to discard them immediately via del info, current, and
• L710 [info/cohesion] src/autoskillit/cli/_marketplace.py: [info] cohesion: Every other terminal in install() builds its result through _typed_result(...), which is also the single place findings are
• L35 [info/cohesion] src/autoskillit/cli/_marketplace.py: [info] cohesion: from autoskillit.cli._install_snapshot import _installed_plugins_json_path as _installed_plugins_json_path is a pure re-exp
• L141 [info/abstraction-surface] src/autoskillit/cli/update/_transaction.py: [info] abstraction-surface: UpdateTransactionResult.install_result (line 141) carries the nested InstallResult through every progress.finish
• L284 [info/abstraction-surface] src/autoskillit/cli/_install_snapshot/_snapshot.py: [info] abstraction-surface: _InstallSnapshot.track_retirement (line 284) is a no-op method - body is a single docstring, _record_id paramete

[Trecek]

[warning] abstraction-surface: UpdateProcessStatus (lines 96-106) is a parallel enum that re-aliases every value as int(InstallProcessStatus.X) - eight of eight members map 1:1 to the public InstallProcessStatus enum defined in _install_contract.py:52-62, and the only production consumer (_update.py:36) compares the result against UpdateProcessStatus.SUCCESS only. The _PROCESS_STATUS_BY_OUTCOME table (lines 109-124) reproduces the same mapping that process_status_for_result(result.install_result) would already express: UpdateTransactionOutcome values are derived inside _map_install_result from InstallOutcome (lines 218-231) and then re-projected onto the identical InstallProcessStatus codes. The two enums and the lookup table form a duplicated policy layer with no observable behavior change - run_update_command could call process_status_for_result(result.install_result) directly and UpdateProcessStatus would vanish.

[Trecek]

This needs a design decision. Direct canonical install-status mapping cannot represent pre-install or upgrade failures where no install_result exists; changing the public boundary requires an explicit contract decision.

[Trecek]

[warning] abstraction-surface: _TransactionProgress (lines 148-189) maintains a strict phase-prepend state machine with two surface fields - phase_history (line 144) and irreversible_pivot_crossed (line 145) - that have zero production readers: grep across src/ for .phase_history and .irreversible_pivot_crossed returns no matches. Every enforcement call (enter(...) lines 257-477) and every record in UpdateTransactionResult carries this state, but the only readers are tests/cli/test_update_transaction.py asserting result.phase_history == UPDATE_TRANSACTION_PHASES. The IRREVERSIBLE_PIVOT_PHASE constant (line 80) likewise has only test-side readers. This is generic state machinery whose invariants cannot be violated by any caller because no caller observes the result - either expose the state to a real consumer (e.g., logging, retry routing) or remove the surface fields, the phase enum, and the progress wrapper in favor of plain function calls.

[Trecek]

This needs a design decision. Transaction history and pivot currently have no production reader, while progress.enter actively enforces phase and finalization ordering; surfacing or removing that audit state changes the contract.

[Trecek]

AutoSkillit PR Review — Follow-up: posting the 5 info-level findings inline as requested. Verdict remains approved_with_comments.

[Trecek]

[warning] abstraction-surface: UpdateProcessStatus (lines 96-106) is a parallel enum that re-aliases every value as int(InstallProcessStatus.X) - eight of eight members map 1:1 to the public InstallProcessStatus enum defined in _install_contract.py:52-62, and the only production consumer (_update.py:36) compares the result against UpdateProcessStatus.SUCCESS only. The _PROCESS_STATUS_BY_OUTCOME table (lines 109-124) reproduces the same mapping that process_status_for_result(result.install_result) would already express: UpdateTransactionOutcome values are derived inside _map_install_result from InstallOutcome (lines 218-231) and then re-projected onto the identical InstallProcessStatus codes. The two enums and the lookup table form a duplicated policy layer with no observable behavior change - run_update_command could call process_status_for_result(result.install_result) directly and UpdateProcessStatus would vanish.

[Trecek]

This needs a design decision. Direct canonical install-status mapping cannot represent pre-install or upgrade failures where no install_result exists; changing the public boundary requires an explicit contract decision.

[Trecek]

[warning] abstraction-surface: _TransactionProgress (lines 148-189) maintains a strict phase-prepend state machine with two surface fields - phase_history (line 144) and irreversible_pivot_crossed (line 145) - that have zero production readers: grep across src/ for .phase_history and .irreversible_pivot_crossed returns no matches. Every enforcement call (enter(...) lines 257-477) and every record in UpdateTransactionResult carries this state, but the only readers are tests/cli/test_update_transaction.py asserting result.phase_history == UPDATE_TRANSACTION_PHASES. The IRREVERSIBLE_PIVOT_PHASE constant (line 80) likewise has only test-side readers. This is generic state machinery whose invariants cannot be violated by any caller because no caller observes the result - either expose the state to a real consumer (e.g., logging, retry routing) or remove the surface fields, the phase enum, and the progress wrapper in favor of plain function calls.

[Trecek]

This needs a design decision. Transaction history and pivot currently have no production reader, while progress.enter actively enforces phase and finalization ordering; surfacing or removing that audit state changes the contract.

[Trecek]

FALLBACK: I was unable to post inline comments (GitHub secondary rate limit blocked the batch POST for 24+ minutes). Posting summary as review body instead. This is a DEGRADED review.

AutoSkillit Review Findings

Verdict: changes_requested (2 critical blocking, 16 warning, 11 info, 1 decision; 30 total after suppressing 3 prior-resolved matches)

src/autoskillit/cli/_install_contract.py

• L1 [info/slop]: Module docstring at L1-L5 ('dependency leaf: performs no I/O and imports only from stdlib') is boilerplate that restates
• L107 [info/defense]: InstallResult.post_init allowlist duplicates _FAILED_STATUS_BY_KIND.keys() at [L107]. Iterate the registry to keep o
• L116 [info/slop]: _FAILED_STATUS_BY_KIND and _OUTCOME_BY_STATUS are two parallel lookup tables; could be unified into a single enum-to-enu
• L116 [warning/abstraction_surface]: Three parallel lookup tables encode install-outcome <-> process-status: _FAILED_STATUS_BY_KIND (L116-120), _OUTCOME_BY_S

src/autoskillit/cli/_install_snapshot/_snapshot.py

• L31 [info/arch]: Two version authorities inside one install transaction. _installed_plugin_root(version=None) falls back to a lazy `fro
• L51 [warning/arch]: New cross-subpackage reach into cli/update/ private internals. _InstallSnapshot.__init__ lazily imports `autoskillit
• L180 [info/slop]: Multi-line comment at L180-L182 in _restore_owned_lease_entry explains the obvious rationale for keeping an inode-locked
• L352 [warning/reachability]: The if lease_entry is None: branch is unreachable. self._lease_path is unconditionally added to self._paths in _init
• L423 [warning/defense]: commit() catches Exception when removing staged directory at [L792]. Broad clause masks ValueError/TypeError/AttributeEr

src/autoskillit/cli/_marketplace.py

• L271 [info/reachability]: _run_claude_admin raises ValueError when argv is empty, but both production call sites pass hard-coded non-empty tuples.
• L281 [info/slop]: _typed_result helper (L281-L299) prints every finding to stdout as a side-effect; tests use capsys to assert findings -
• L362 [warning/arch]: _verify_cleanup re-implements the nested settings.json hook-traversal that _hooks._evict_stale_autoskillit_hooks own
• L414 [info/slop]: Old multi-paragraph prose about a dangling-pointer incident was deleted from _InstallSnapshot docstring at L243-L247 but
• L509 [critical/bugs]: The new install transaction removed the with _InstallLock(): wrapper that previously serialized concurrent installs. _
• L721 [critical/defense]: _compensated_result assigns failure_kind=primary.kind. If primary.kind is InstallFailureKind.ROLLBACK, the resulting Ins
• L721 [warning/defense]: install() catches BaseException at [L1676] but _compensated_result can itself raise (e.g., from _restore_owned_lease_ent

src/autoskillit/cli/update/_transaction.py

• L56 [info/slop]: MAINTENANCE_EXTRAS module-level dict at L56-L59 holds AUTOSKILLIT_SKIP* keys consumed only inside build_maintenance_en
• L113 [warning/slop]: PROCESS_STATUS_BY_OUTCOME table reproduces the InstallProcessStatus mapping already expressible through process_status
• L165 [warning/bugs]: _TransactionProgress.enter(RESULT_FINALIZATION) only guards against double-finalization; it does NOT enforce that all pr
• L196 [info/slop]: _process_finding and _upgrade_failure are tiny shims used once each in the same file - inlining both would shrink the fi
• L482 [warning/defense]: UpdateTransactionResult.verified_identity is populated from two sources depending on completion path: install_result.ver

src/autoskillit/core/_claude_env.py

• L168 [warning/defense]: is_protected_maintenance_extra at [L3140-L3151] protects only AUTOSKILLIT{BACKEND,SESSION,CAMPAIGN,ORDER}. An attacker

src/autoskillit/workspace/_install_state.py

• L232 [warning/arch]: Duplicated contract expressed as a bare string literal across a module boundary. `_record_matches_current_installed_arti

src/autoskillit/workspace/_installed_artifact.py

• L110 [warning/slop]: from_managed_root reconstructs home via positional parents[5] rule - duplicates the managed-root layout that _install_sn
• L180 [info/arch]: IL-1 workspace diagnostics embed CLI operator instructions (e.g., 'Run autoskillit install to reinstall the plugin.')
• L312 [warning/reachability]: Defensive post-read re-validation `if identity.semantic_key != spec.semantic_key or identity.managed_path != spec.manage
• L355 [warning/defense]: The no-create fast path at [L3761-L3766] uses is_symlink() OR exists() - a dangling symlink returns True from is_symlink
• L355 [warning/defense]: _validate_supplied_lease catches (OSError, RuntimeError, ValueError) at [L3736] and reduces them to a single string find
• L355 [warning/defense]: verify_installed_plugin_artifact catches BaseException to release the lease. While it re-raises at [L3853], pairing a Ba

tests/cli/test_doctor.py

• L24 [info/slop]: _filesystem_snapshot helper at L24-L40 is a once-used fixture-style helper for a single parametrized test; same logic re

[Trecek]

Resolved the findings from review 4824384286. GitHub placed that review in the top-level body after the inline batch hit a secondary rate limit, so this is the corresponding consolidated response.

Addressed 7 findings:

• Centralized the unreadable installed-artifact check name (79f771d).
• Moved fetch-cache path policy to the shared install-snapshot layer (5b617f7).
• Shared hook cleanup traversal between eviction and verification (74edd47).
• Derived failed-kind validation and reverse failed-status entries from one mapping, addressing two findings (16e0a7b).
• Removed redundant installed-artifact identity revalidation (3f67834).
• Preserved KeyboardInterrupt/SystemExit when compensation itself fails, with regression tests (257e16c).
• Updated the existing line-based JSON-write allowlist after the helper move (80535c0).

I also investigated all 23 remaining findings and did not change code where the claim did not hold:

Rejected findings and evidence

1. Workspace installation guidance is the intentional domain diagnostic surfaced unchanged by multiple callers.
2. The install transaction always derives its target from explicit expected_version; there is no second transaction authority.
3. RESULT_FINALIZATION is intentionally valid after every operational prefix, as enforced by transaction-progress tests.
4. install() still holds _InstallLock across staging, leases, child commands, publication, and verification; _lock_owned only prevents nested acquisition.
5. Update-saga outcomes such as FAILED_UPGRADE are not representable by the install-result mapping.
6. The rollback comment documents the non-obvious inode-descriptor versus lexical-path distinction.
7. The dependency-leaf docstring records a useful enforced architecture boundary.
8. Moving managed-root inversion into the IL-3 CLI snapshot module would violate the import-layer boundary; InstallStateSpec.from_managed_root is already the validated inverse.
9. The alleged relocated incident narrative is absent from the current marketplace file.
10. _typed_result is the CLI presentation boundary; printing findings is required and tested behavior.
11. The doctor and install-transaction filesystem helpers have different snapshot contracts and test domains.
12. Core owns the allowed-key taxonomy; _MAINTENANCE_EXTRAS owns this caller's choice to set both guards.
13. _upgrade_failure has multiple callers, and _process_finding serves both child processes.
14. The staged-lease fallback converts future invariant corruption into recovery diagnostics instead of a secondary unpacking failure.
15. Rejecting empty argv is a valid subprocess boundary invariant.
16. Only the two install-contract mapping directions overlap; the update mapping belongs to a separate domain.
17. No production _compensated_result caller supplies ROLLBACK as the primary failure kind.
18. The dangling-symlink branch was read in reverse: lexical evidence prevents the no-create return and causes lease acquisition.
19. Expected lease failures intentionally normalize into structured findings; no consumer branches on the original exception type.
20. Verifier-owned leases must close for control-flow exceptions too, and close_preserving retains the primary outcome.
21. Broad staged-directory cleanup suppression is deliberate post-commit finality with regression coverage.
22. Failure paths preserve available child identity evidence, while successful registered-plugin paths require fresh verification; those sources are intentionally lifecycle-specific.
23. Unsupported AUTOSKILLIT extras, including RUNTIME and DEBUG, are rejected by the independent two-key allowlist.

Validation is green at head 80535c0: 32,853 passed, 570 skipped, 27 expected failures; pre-commit run --all-files also passed. The four older unresolved inline discussion threads already contain prior autoskillit resolution replies and were left untouched by the idempotency guard.