Rectify: Skill-Contract Immunity for Pre-Existing Artifacts (Forward Version-Skew) - #4474
Open
Trecek wants to merge 24 commits into
Open
Rectify: Skill-Contract Immunity for Pre-Existing Artifacts (Forward Version-Skew)#4474Trecek wants to merge 24 commits into
Trecek wants to merge 24 commits into
Conversation
Replace SkillInfo.invalid_reason (a free-form accumulated string) with SkillInfo.invalidities: tuple[SkillInvalidity, ...], where each entry carries a typed SkillInvalidityKind plus its detail message. invalid_reason becomes a derived property joining invalidities with "; " — byte-identical to the old accumulated string, so every existing string reader keeps working unchanged. SkillInvalidityKind (core/types/_type_enums.py) enumerates every current invalid_reason producer: FRONTMATTER_PARSE, FIELD_SHAPE, RESERVED_FIELD, UNKNOWN_CAPABILITY, UNDECLARED_CAPABILITY, SEMANTIC_UNDECLARED_TOKENS, SEMANTIC_MISSING_VERSION, SEMANTIC_VERSION_MISMATCH, SEMANTIC_PLAN_INVALID. This makes the contract surface mechanically enumerable — the precondition for the forcing-function remediation registry in a later step. parse_skill_semantic_plan now returns (kind, message) pairs instead of bare strings so its diagnostics can be typed at the source. Migrate every invalid_reason= construction site (frontmatter-parse early return, __post_init__ self-derivation, the accumulated-reasons build in _skill_info_from_frontmatter, its authenticity-diagnostics replace() call, and _llm_triage.py's invalid_reason=None reset) to build/reset invalidities instead. replace() with invalid_reason= would now raise TypeError since it is a property, not a field. SkillCatalogEntry keeps its own plain invalid_reason field untouched — catalog entries are guaranteed valid by construction, so it needs no migration. Ref: GitHub issue #4470. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…2.2, 2.4) Resolver fall-through (recipe-loader semantics): - resolve_effective now iterates project-local candidates in precedence order and returns the first VALID one, logging and skipping invalid higher-precedence candidates instead of letting them shadow a valid bundled twin or a valid lower-precedence local copy. Falls through to the bundled source when no project-local candidate is valid; returns the highest-precedence invalid candidate only when nothing anywhere is valid, so callers can still report why a configured name failed. - resolve_local_candidate preserves the OLD resolve_effective's first-path-match-regardless-of-validity semantics for _llm_triage, the one caller whose purpose is comparing raw, possibly-stale, on-disk content against a stored baseline. - _list_effective_unfiltered now returns (tuple[SkillInfo, ...], tuple[SkillExclusion, ...]): a name is claimed by a project-local search dir only on successful parse (the recipe-loader "seen"-on-success rule), deleting the old clobber where an invalid shadowing copy replaced a valid bundled entry. Invalid candidates produce a SkillExclusion record instead of raising or silently vanishing. - list_effective attaches exclusions to EffectiveSkillCatalog and narrows its remaining catalog-wide raise to bundled/extended-source entries only — invalid project-local candidates no longer reach it. - validate_skill_tier_roles' invalid-tier message now includes the file path and per-kind remediation hints when the concrete resolver supplied a real SkillInfo. - Fixed a latent bug in resolve_invocation's pack-expansion: it called _list_effective_unfiltered but iterated the old single-tuple return as if it still were one, which would have raised AttributeError for any skill with a PACK_REGISTRY-member activate_deps. New IL-0 protocols (core/types/_type_protocols_workspace.py): SkillInvalidityAuthority and SkillExclusionAuthority cross the IL-0 boundary the same way SkillFrontmatterAuthority already does; EffectiveSkillCatalogAuthority gains an `exclusions` field. Threaded through the two catalog-reconstruction sites that would otherwise silently drop the records: compile_session_skill_catalog (workspace/session_skills.py) and the projected-artifact authority's namespace_sources rebuild (workspace/_projected_artifact/authority.py). Remediation registry (core/types/_type_constants.py), the forcing function (issue remediation item 2, second half): SKILL_CONTRACT_REMEDIATIONS maps every SkillInvalidityKind to a SkillContractRemediationDef — introduced_in, a DETERMINISTIC/ADVISORY RemediationAction, and a hint — modeled on RETIRED_INSTALL_ARTIFACT_SHAPES, guarded by an append-only-completeness assertion at import time. UNDECLARED_CAPABILITY, SEMANTIC_MISSING_VERSION, and SEMANTIC_UNDECLARED_TOKENS are the three kinds a future SkillMigrationAdapter can repair deterministically; the rest are advisory-only. Documented the forcing function in AGENTS.md §3.1 alongside the Hook/Skill-rename entries it's modeled on. Ref: GitHub issue #4470. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Wrap the three composition roots' validate_skill_tier_roles/list_effective calls in try/except SkillContractError instead of letting them crash with a raw traceback (issue remediation item 3): - cli/session/_session_cook.py (cook): catches around both the tier-role validation and the SESSION-role list_effective call (they're not adjacent in this function — onboarding/backend-resolution sits between them), prints a clean message plus a doctor pointer, exits non-zero. - cli/session/_session_order.py (order): same treatment, adjacent block. - server/_factory.py (make_context): logs a structured error event and re-raises as-is — every MCP-facing caller already wraps composition in try/except SkillContractError and renders its own envelope, so the message just needs to stay actionable (already true after 2.2). On success, all three log/print one line per non-empty catalog.exclusions (path + hints) via two new shared helpers in cli/session/_session_launch.py: render_skill_contract_composition_failure and render_skill_catalog_exclusions. The five other crash sites named in the issue (_fleet_run.py, skill_projection.py, _prompts.py, _serve_helpers.py, tools_fleet_dispatch.py) need no guards — after 2.2 they can no longer raise for project-local staleness, and genuine packaging errors should still fail loudly there. Updates the one pre-existing test whose pin this consciously changes: test_cook_rejects_orchestrator_skill_in_l1_tier_before_launch moves from pytest.raises(SkillContractError) to pytest.raises(SystemExit) plus an output assertion (no Traceback, mentions the skill name and required role). Ref: GitHub issue #4470. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
For a local-only invalid skill (the one case where an invalid SkillInfo still deliberately escapes resolve_effective by design), these four sites previously branched only on `is None` and silently consumed the invalid candidate's content with no warning: - recipe/_skill_helpers.py:_resolve_skill_md — now treats invalid as unresolved (returns None, the existing not-found branch) and logs the invalidity with remediation hints. - recipe/_contracts_staleness.py:check_contract_staleness — now skips hashing an invalid candidate (treats it as absent, current_hash="") instead of hashing whatever invalid content is on disk, and logs. - core/types/_type_helpers.py:resolve_target_skill — IL-0, so the guard uses only the invalid_reason field already visible on the Protocol (no new imports): an invalid candidate now renders as unresolved instead of resolving its (possibly wrong) source. - cli/doctor/_doctor_config.py:standing_backend_pins_feasibility — now reports the invalidity as an explicit WARNING finding instead of silently skipping the semantic-plan feasibility check for that skill. Extracted invalidity_hints() (workspace/skills.py) — the per-kind remediation-hint lookup used by SkillExclusion.from_skill_info and validate_skill_tier_roles (2.2) — into a shared helper so these new call sites (and 2.6's doctor check) don't reimplement it. Ref: GitHub issue #4470. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SkillMigrationAdapter (migration/engine.py), a DeterministicMigrationAdapter registered in default_migration_engine(): - discover(project_dir): globs SKILL.md under all four ALL_PROJECT_LOCAL_SKILL_SEARCH_DIRS. - needs_migration(file): resolves the raw candidate via resolve_local_candidate() (not resolve_effective — which would now fall through to a valid bundled twin instead of validating the stale file itself) and returns true iff at least one invalidity kind is registered DETERMINISTIC in SKILL_CONTRACT_REMEDIATIONS. - migrate(file): applies every applicable deterministic remediation. UNDECLARED_CAPABILITY inserts the missing capability name(s) parsed from the authenticity diagnostic. SEMANTIC_MISSING_VERSION stamps semantic_version. SEMANTIC_UNDECLARED_TOKENS drops a retired capability name from uses_capabilities and stubs its semantic_requirements replacement field — the only half of that kind fixable without touching body text; a raw portable token (Agent(, subagent_type=, …) literally present in prose cannot be repaired frontmatter-only, so that half stays advisory in practice despite the registered action, with the hint still pointing at the fix. Frontmatter is re-serialized as a whole; body is carried through byte-for-byte from the parsed result. - Raises on an unrecognized deterministic kind (the guard T8 exercises). Exported RETIRED_SEMANTIC_CAPABILITIES (was module-private in skill_capabilities.py) so the adapter doesn't duplicate the retired-capability-to-semantic-field mapping. CLI driver (cli/app.py): the `migrate` command was hardcoded to recipes and never touched MigrationEngine, so a registered skill adapter stayed unreachable. Added a skill pass (_migrate_skills): reports every pending skill's path, invalidity kinds, and hints unconditionally; a new `--fix` flag (default off, preserving the pinned read-only contract in tests/cli/test_install.py) applies migrate() per file via the engine and prints per-file fixed/FAILED results. `--check`'s exit-code gate stays scoped to recipes, unchanged. Documented in docs/cli.md. Ref: GitHub issue #4470. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Added DefaultSkillResolver.scan_effective() — a public pair-returning wrapper around _list_effective_unfiltered, giving doctor (and other operator-facing tooling) an entry point that doesn't reach for the underscore-prefixed internal implementation. _check_project_local_skill_contracts (cli/doctor/_doctor_skills.py, registered as check 41): reports every SkillExclusion from scan_effective() — path, invalidity kinds, remediation hints, and an `autoskillit migrate --fix` pointer when at least one kind is DETERMINISTIC. Report-only, consistent with doctor's no-writes contract (test_doctor_readonly.py). This is the check that makes the "silently dropped user-native skills" erosion visible for project-local skills: resolution-boundary containment already prevents the crash/clobber, but an excluded file's only trace before this was a log line nobody reads. The analogous warn-and-drop of invalid ~/.codex/skills profile entries (session_skills.py, THIRD_PARTY source, outside DefaultSkillResolver) is an explicit non-goal here — it's a user-global profile directory, not a project artifact, and keeps its existing behavior. Ref: GitHub issue #4470. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Test-first coverage for the resolution-boundary containment, remediation registry, migration adapter, doctor check, and silent-degrade hardening implemented in the preceding commits (2.1-2.7). Written after the implementation given the "run no test runner" constraint on this skill — each test's correctness is reasoned against the concrete new/changed behavior it targets, not verified red-then-green locally; the orchestrator's test-check gate is authoritative. T1-T4 (tests/workspace/test_skills.py): the #4470 composition reproduction (stale project-local audit-bugs falls through to its bundled twin, one exclusion recorded); a local-only invalid skill excluded with a record; multi-dir fall-through preserves recipe-loader semantics; replaces the fail-closed pin with a fall-back-with-exclusion-record pin, extends the local-only pin to assert the exclusion. T5 (tests/cli/test_cook_profile.py): a tier-configured skill invalid everywhere reports its path, a remediation hint, and an `autoskillit doctor` pointer — clean SystemExit, no traceback. T6 (tests/server/test_factory.py): the real make_context() composition path survives a stale shadowing skill and logs the exclusion. T7-T9 (new tests/contracts/test_skill_contract_remediations.py + tests/contracts/fixtures/skill_contract_corpus/): bundled/repo-local skill hygiene pinned at merge time; every SkillInvalidityKind has a registered remediation (with a teeth-test proving the DETERMINISTIC- coverage guard actually rejects an unhandled kind); a three-fixture historical corpus that must validate cleanly or migrate cleanly. T10 (tests/migration/test_engine.py, tests/cli/test_install.py): SkillMigrationAdapter discover/needs_migration/migrate behavior, plus the CLI reachability tests — default `migrate` reports without touching the file, `migrate --fix` rewrites it. T11 (tests/cli/test_doctor.py): the new project-local skill contracts doctor check reports both a shadowing and a local-only invalid skill with kinds/hints/fix-pointer, and passes clean on a clean project. T12 (tests/recipe/test_rules_skill_content.py, test_contracts.py, tests/cli/test_doctor_standing_pins.py, tests/contracts/test_target_skill_invocability.py): all four hardened silent-degrade sites now treat a local-only invalid candidate as unresolved/absent instead of silently consuming it. T13 (tests/workspace/test_session_skills_allow_only_and_closure.py): pack expansion survives an unrelated invalid project-local skill in the same _list_effective_unfiltered scan — pins the skills.py pack-expansion unpacking fix across the 2.2 signature change. T14 (tests/test_llm_triage.py): triage still receives the raw, stale project-local content via resolve_local_candidate for a skill shadowing a valid bundled twin, not the bundled twin resolve_effective's fall-through would otherwise silently substitute. Ref: GitHub issue #4470. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hment - SkillMigrationAdapter.migrate(): the SEMANTIC_UNDECLARED_TOKENS branch now detects the case where no declared retired capability was found (the only trigger was a raw literal token in the body, which this frontmatter-only adapter never edits) and returns success=False instead of silently claiming a fix that never happened. - workspace/skills.py: the three project_local_skill_rejected warning call sites (resolve_effective, resolve_local_candidate, _list_effective_unfiltered) now carry hints=invalidity_hints(...), per plan step 2.4's wiring list. - validate_skill_tier_roles: the upgraded error message now also names the bare invalidity kind(s), alongside path/hints/doctor pointer. - cli/app.py _migrate_skills(): --fix now prints per-file results — which invalidity kinds a successful deterministic fix addressed, plus hints for any ADVISORY kinds left unaddressed on the same file. Addresses items 1-4 of the consolidated audit-impl remediation task list.
- missing_semantic_version.md: add the missing logical_roles declaration for 'researcher' so the fixture's only defect is the intended missing semantic_version (it previously carried an unrelated second SEMANTIC_PLAN_INVALID defect the migration adapter isn't meant to fix). - test_engine.py: test_migrate_no_op_when_nothing_deterministic had an orphaned assertion (asserting MIGRATE_RECIPES_MAX_RETRIES, an LLM-recipe- migration constant unrelated to this deterministic, non-retrying adapter) glued onto its end by a bad insertion splice. Removed it, and restored it to its actual owner — test_failed_headless_retries_match_constant, whose name promises exactly this assertion and was the one left without it. Addresses items 5-6 of the consolidated audit-impl remediation task list.
…d skills Three of the four Step 2.7 silent-degrade guard tests shadowed a real bundled skill (resolve-failures, investigate, open-kitchen). Post-2.2, resolve_effective correctly falls through to the valid bundled twin for a shadowed name, so the invalid_reason is not None guard at each site never fired — one test failed outright, one was vacuous (verified by temporarily reverting the guard clause and confirming it still passed), and one only coincidentally matched. Replaced all three with a fabricated, local-only skill name that has no bundled twin anywhere — the only scenario in which resolve_effective still returns an invalid SkillInfo post-2.2, and the actual scope Step 2.7's own framing describes. Each replacement is empirically verified to discriminate guard-present from guard-absent behavior by temporarily reverting the guard and confirming the test fails: - test_doctor_standing_pins.py: renamed to test_invalid_local_only_skill_reports_invalidity_instead_of_silent_skip; authors a minimal project-local recipe whose one step targets the fabricated skill name, reached via agent_backend.recipe_overrides. - test_contracts.py: current_value expectation corrected to '' (the actual, verified behavior — the hash-mismatch branch never fires for an empty/absent current_hash, so a local-only invalid override yields an empty stale list rather than a StaleItem). - test_target_skill_invocability.py: input skill_command chosen so the guard-present and guard-absent renders visibly diverge (autoskillit: sigil preserved vs. stripped to the PROJECT_LOCAL namespace). Addresses items 7-9 of the consolidated audit-impl remediation task list.
…r change - test_project_local_overrides.py: test_prepare_skill_projection_authenticates_project_root_not_managed_add_dir pinned the old fail-closed behavior (pytest.raises(SkillContractError, match='effective skill catalog contains invalid contracts')) for a second, independent crash site (skill_projection.py:239 via prepare_skill_ projection) — one of the plan's five 'no guard needed' sites, which now falls through to the valid bundled process-issues twin instead of raising. Updated to assert the projection succeeds using bundled content, with an exclusion recorded, mirroring how T4 handled the analogous case in test_skills.py. - test_type_protocol_shards.py: test_workspace_shard_all pins the exact protocol-name set exported from _type_protocols_workspace.py. Added the two new IL-0 protocols this implementation correctly introduced — SkillExclusionAuthority and SkillInvalidityAuthority — both structurally necessary per plan step 2.2's IL-0 boundary-crossing paragraph. Addresses items 10-11 of the consolidated audit-impl remediation task list.
…fication sweep test_order_rejects_orchestrator_skill_in_l1_tier_before_launch pinned a raw SkillContractError propagating out of cli.order(...), but Step 2.3's composition-root rendering change (_session_order.py) already catches SkillContractError around validate_skill_tier_roles and exits cleanly — exactly the same change T5 already accounted for on the cook path (test_cook_rejects_orchestrator_skill_in_l1_tier_before_launch), just never mirrored for order's own pin. Confirmed via git stash that this failure predates every change in this remediation pass (reproduces identically on the unmodified branch). Updated to the same pytest.raises(SystemExit) + output-assertion pattern as the cook pin. Found via the broader tests/cli/, tests/workspace/, tests/recipe/, tests/contracts/, tests/migration/, tests/core/, tests/server/ sweep run to verify the 11 listed remediation items — outside the original 11-item list but a real, confirmed regression on this branch that task test-all would otherwise catch.
Trecek
force-pushed
the
impl-rectify_skill_contract_immunity-20260805-144018
branch
from
August 6, 2026 16:55
01a841a to
46f54e8
Compare
Trecek
enabled auto-merge
August 6, 2026 22:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A stale, pre-contract-era copy of the bundled
audit-bugsskill in an external user repo crashes every AutoSkillit entry point because invalid project-local skill candidates escape the resolution boundary and enforcement is smeared across at least twelve downstream consumers — eight of which hard-raiseSkillContractErroruncaught, and four of which silently consume the invalid content. This is the third manifestation of skill-contract changes shipping without a migration or tolerance path for pre-existing artifacts.Closes #4470
Implementation Plan
Plan file:
.autoskillit/temp/rectify/rectify_skill_contract_immunity_2026-08-05_133349.md🤖 Generated with Claude Code via AutoSkillit