feat(skills): atomically promote safe auto-skill updates

[rubencu]

Problem / Motivation

When skills.approval_required=false, newly generated auto-skills could be applied automatically, but safe prose-only updates to existing auto-skills still required manual approval. The pending-candidate flow also inspected public candidate directories before consuming them, allowing concurrent replacement or mutation between validation and promotion.

Why it matters

Users who opt out of approval should receive safe prose improvements consistently without giving script-bearing changes unattended execution. Promotion must consume exactly the candidate that was validated; otherwise concurrent writers or hostile filesystem entries can redirect, corrupt, misroute, or lose pending work.

What changed (motivation → approach → change)

• Auto-applies existing-skill updates only when approval is disabled, auto-refine is off, and the candidate is prose-only. Any candidate containing scripts remains staged for mandatory human review.
• Makes the first candidate operation an atomic same-filesystem rename into a random private claim, after first proving no-replace rename support. Metadata, layout, scripts, version, and body are inspected only from that claim; stale dashboard new/update routing is revalidated there before mutation.
• Adds bounded cross-process namespace, target, and per-claim locks with a documented hierarchy. Every app-owned live auto-skill mutation shares the target lock, and dashboard waits run outside the asyncio event loop.
• Refuses the bare reserved namespace and every case-folded spelling (auto, AUTO, Auto, auto/<slug>, and aliases) before generic mutation, preventing recursive namespace deletion and case-insensitive macOS/Windows aliases from bypassing the live auto-skill target lock.
• Binds unattended promotion to the exact staged bytes, target, base version, original live-body digest, and stage identity. Legacy human-approved candidates without base_version remain supported.
• Authenticates completed claim state in the held lock file outside the removable claim tree, so failed marker repair or partial cleanup cannot restore consumed bytes. Refused claims are restored without overwriting same-slug replacements, and abandoned legacy marker-only claims remain recoverable.
• Keeps private claim/lock roots on the permanent agent-deny floor. Candidate trees must contain only real directories and lone regular-file inodes: links, hardlinks, special files, traversal failures, and stat failures reject and restore the claim. Lock files retain no-follow, regular/single-link, and pre-open/opened-inode authentication.
• Adds atomic no-replace publication/restoration support through platform_compat for Linux, macOS, and Windows, including older-glibc Linux fallback and Windows lock preparation.
• Strengthens summary freshness from mtime alone to an optional (mtime, message-byte-count, rotation-generation) identity. Metadata-only rewrites remain valid, same-tick message changes are detected, and legacy sidecars without size retain their prior semantics.
• Redirects repository compileall bytecode into pytest's temporary directory, keeping parallel tests hermetic and preventing checkout races or generated vendor files.
• Updates configuration help, dashboard notifications, security path handling, and the auto-skill/session-summary system specifications.

Tests

• Canonical full backend suite passed on exact final head 25ac3513315f1db049d8b10fbdc590828ac4daa2: 59,035 tests collected, exit 0 with 16 workers after removing only the injected xdist worker variable inside Python.
• Earlier broad changed-surface backend validation passed 1,113 tests with 23 skipped across claim/versioning, pending API/notification/update flows, history signatures, platform compatibility, security, and agent-home/compileall isolation. After the two final security fixes, focused attack-path selection passed 16 tests, the complete skill-versioning module passed 95 tests, and the pending-skill module passed 29 tests before the conflict-free rebase; the exact rebased head is covered by the canonical suite above.
• Added deterministic regressions for unsupported no-replace filesystems, claim isolation, candidate replacement, stale kind routing, same-target and live-mutator serialization, bare and mixed-case reserved-namespace aliases, exact staged-byte binding, abandoned recovery, authenticated completion after partial cleanup, post-claim hardlink mutation, fail-closed inode-stat errors, link/junction/hardlink defenses, private-root containment, lock inode swaps, same-tick history changes, and cross-platform no-replace behavior.
• On the exact final head, Black ratchet over 21 changed files, isort, changed-file Flake8, mypy over 1,003 source files, focus-cue self-test (30 probes) and exact-base scan, diff_signals.py, and git diff --check pass.
• The broader policy profile also passed brand/harness/changelog self-tests and diff scans, docs lint/self-test over 215 files, per-file coverage self-test, vendor manifest over 46 files, scrub lint, pinned cfn-lint==1.22.3, and the complete frontend build/type/lint/i18n/render/test profile; this PR changes no frontend files.

Manual verification

N/A — there is no rendered UI change; promotion, dashboard routing, concurrency, recovery, summary freshness, and platform behavior are covered by deterministic automated tests.

Related Issues

Fixes #3795

Checklist

• Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[rubencu]

GPT review dispositions

Exact reviewed head: 484dd701eb08a6763e768ae901a85703bd1cf516.

Finding	Disposition	Resolution
Windows lock preparation could fail on an empty file	Accepted / fixed	Authenticate the descriptor first, then seed one inert byte before msvcrt.locking; platform regression added.
Older glibc may lack the exported renameat2 wrapper	Accepted / fixed	Added the native Linux syscall fallback on supported architectures while preserving atomic no-replace semantics.
Symlink/junction claims could be traversed	Accepted / fixed	Treat linked claim paths as opaque names for restoration/removal; never inspect metadata, markers, or descendants through them.
Valid non-object JSON metadata could be rewritten	Accepted / fixed	Preserve original metadata bytes on refusal/recovery; regression verifies byte-identical restoration.
Claim failure during unattended apply could suppress review notification	Accepted / fixed	Re-emit the staged event when the original suppressed candidate remains public.
Private-root sensitivity checked only one path spelling	Accepted / fixed	Require both logical and resolved private roots to be agent-denied before claiming.
Live mutations outside promotion could bypass the target lock	Accepted / fixed	All app-owned create/edit/refine/pin/injection/archive/restore/delete paths now share the target lock.
Dashboard lock waits could block the asyncio loop	Accepted / fixed	CRUD, approval, dismissal, listing, discovery, and consolidation callers offload bounded lock waits to executors/threads.
Same-target and live-mutator races lacked deterministic proof	Accepted / fixed	Added threading.Event-ordered serialization regressions without sleeps or retries.
.promoted handling could follow a linked claim parent	Accepted / fixed	Marker inspection and removal are skipped entirely for linked/junction claim parents.
Partial recursive cleanup could remove the marker, fail later, and make consumed work recoverable	Accepted / fixed	Completed-claim cleanup recreates and authenticates the marker before releasing the claim lock whenever residue remains.
Unattended binding hashed normalized text rather than exact stored bytes	Accepted / fixed	Bind from SKILL.md.read_bytes(); simulated Windows newline regression verifies CRLF-safe identity.
Pre-planted linked private descendants could redirect claims or locks	Accepted / fixed	Authenticate .private, claims, locks, and locks/claims as real non-linked directories contained beneath the resolved private root.
Lock files could be followed, hardlinked, or inode-swapped	Accepted / fixed	New files use exclusive creation/no-follow; existing files require pre-open and opened-inode identity plus regular/single-link checks.
Stale dashboard new/update routing could publish a replaced update as a new skill	Accepted / fixed	New-skill promotion re-reads kind from the private claim before mutation, refuses claimed updates, restores exact candidate bytes, and retains legacy missing/non-object metadata compatibility.

Validation on this exact head: complete affected suite 1015 passed, 20 skipped, 49 warnings; Black, isort, Flake8, mypy (988 source files), compilation, docs checks, and git diff --check pass. Fresh GPT and Opus exact-head reviews both report no findings.

[rubencu]

GPT review dispositions for c970b8875168f1856a23f5748d7d0399bd12b5eb

• Bare reserved-namespace spellings could reach generic recursive deletion — fixed in 25ac3513315f1db049d8b10fbdc590828ac4daa2.

  _live_auto_mutation_lock now partitions the requested name and refuses any first path segment whose case-folded value is auto, including the bare namespace before generic unlocked mutation can run. test_delete_refuses_bare_reserved_auto_namespace verifies auto, AUTO, Auto, and aUtO all return false, never invoke the recursive helper, and preserve a live skill.

• A public hardlink alias could mutate a candidate after it was privately claimed and reviewed — fixed in 25ac3513315f1db049d8b10fbdc590828ac4daa2.

  Candidate validation now requires real directories and lone regular-file inodes (st_nlink == 1) and fails closed for links, special files, traversal failures, and lstat errors at pending-detail, claim-layout, and live-target checks. test_approval_refuses_candidate_mutated_through_hardlink_after_claim creates a real same-filesystem hardlink, mutates through it after claim, and verifies approval is refused with no live publication; test_candidate_inode_guard_fails_closed_on_stat_error verifies stat failures also refuse inspection.

Exact-head evidence: test/test_skill_versioning.py 95 passed; focused namespace/link/symlink selection 16 passed, 79 deselected; canonical suite collected 59,035 tests with exit 0; Black, isort, changed-file Flake8, full mypy (1,003 source files), focus checks, and git diff --check passed. Fresh pinned GPT and Opus local reviews both report no findings on 25ac3513315f1db049d8b10fbdc590828ac4daa2.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR surfaced in automated triage with merge conflicts and blocking readiness items. I assessed it, and it is outside what unattended automation should drive:

• At ~3,300 added lines introducing a new cross-process locking/claim subsystem (namespace, target, and per-claim lock hierarchy across skills.py and security.py, plus the prose-only auto-apply policy), it exceeds this pipeline's size threshold for automated fixes — too much surface to reason about safely without the author.
• The branch conflicts with main; resolving conflicts across this surface requires your design intent.

When you've rebased and resolved the conflicts, the pipeline will re-assess on its next cycle.

[rubencu]

CI triage for head f7327c0: 59/61 green. The two remaining failures:

1. Backend Tests (Windows) (3) — flake, not PR-caused: test_session_control.py fails with SessionControlError: too many sessions created recently; retry shortly (rate-limiter state bleeding between tests on the runner). Both tests pass locally on this head, this PR touches no session-control code, and the same shard was green on the previous round (8b99ec4) whose only delta vs this head is one deleted coverage-baseline line. A re-run of the failed job should clear it.

2. Fork workflow-change guard — this PR necessarily edits two .github/ baseline text files: black-baseline.txt (Black ratchet: the diff-touched files were reformatted, so their baseline entries must drop) and coverage-baselines/backend.txt (Coverage Gate instructed removing the prompts.py entry after it cleared the 80% floor). Needs the allow-fork-workflow-change label per the guard's message.

Coverage Gate itself now passes. No code changes pending on my side.

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 1d24654faf935ba4b5c6cf89418bc3f5319d7baa via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: CONCERNS

stage_token is a second, weaker spelling of the byte binding: it is written to the attacker-readable public .meta.json, so it authenticates nothing the binding doesn't.

Not justified as shipped

• Item 9 — rides along: session-summary cache freshness is a separate subsystem's job in a skills-promotion PR (declared, and backed by tests that fail on base).
• Item 10 — undeclared: staging a 51st same-slug candidate now returns failure instead of a name that was never written; nothing in the description mentions it.

What this change ships

Intent: let users who disabled skill approval receive safe prose-only updates automatically, while making candidate promotion consume exactly the bytes that were validated — an ADDITION carrying a TOCTOU FIX (linked issue #3795).

1. Prose-only updates to live auto-skills go live unattended, with a rollback snapshot, when approval is off — justified
2. Script-bearing or auto-refine-covered updates still stage for human review — justified
3. A "Skill auto-updated" notification and WS event fire when an unattended update lands — justified
4. Approve/dismiss now consume a private claimed snapshot; a concurrent same-slug replacement can no longer be promoted, corrupted, or lost — justified
5. Hosts whose filesystem lacks atomic no-replace rename now refuse promotion/dismissal; candidates stay staged (probed at runtime, fail-closed) — justified
6. Every live auto-skill mutation now waits up to 10s on a cross-process per-target lock — justified
7. Case-folded / trailing-dot aliases of the reserved auto namespace are refused for mutation — justified
8. New always-hidden data-home leaf skills/auto/.private (sandbox mask + agent-deny floor) — justified
9. Session-summary caches gain a size field so a same-mtime-tick append invalidates them — rides along
10. Staging a 51st same-slug candidate reports failure instead of claiming success — undeclared
    (More than 10 differences exist — the compileall bytecode redirect, config help text, and two platform_compat helpers are the omitted, smaller ones; each has counted consumers.)

Watch

The unattended path runs two identity checks: expected_stage_token (random hex, compared against stage_token read from claimed .meta.json) and expected_candidate_binding (SHA-256 over the staged bytes + target + base_version + base_content_hash). The token is persisted in the publicly writable pending dir's .meta.json (skills.py staging hunk), so any writer able to replace the candidate can copy it; every tamper the token refuses, the binding refuses too. One check is the subtraction.
Clears when: the token plumbing is deleted, or the author names one concrete tamper the binding accepts and the token rejects.

Subtractions

• Drop stage_token/expected_stage_token end to end — _stage_skill_update (history_consolidation.py), stage_skill_candidate, auto_apply_pending_update, _approve_claimed_update_locked (skills.py) — and keep only expected_candidate_binding; the binding hashes the exact staged bytes plus target/base identity, which is the whole guarantee the token gestures at.

[FIRST-PRINCIPLES-REVIEWED] 1d24654

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 1d24654faf935ba4b5c6cf89418bc3f5319d7baa via the fork AI-review pipeline; updated in place on each push.

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/skills.py:5269 -- attended candidates remain mutable through pre-claim descriptors
attended_backup = self._validate_and_redact_candidate(src, target_name) / candidate_body = (src / "SKILL.md").read_text(...)
Agent-held descriptor -> validation completes -> bytes change -> unvalidated prompt content goes live.
Anchor: backend-security-controls
Fix: Promote fresh private inodes created from the validated bytes.

FINDING -- src/kiro_crew/history_consolidation.py:1701 -- lifecycle failure after self._last_lifecycle = _time.time() suppresses retries for an hour -> Fix: update the timestamp only after run_skill_lifecycle succeeds.

[BLOCK-MERGE] 1d24654
[GPT-REVIEWED] 1d24654

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

The adjudicable block is empty; only fenced finding F1 requires a ruling.

F1 — attended-path TOCTOU on claimed candidate.

The attended promotion branch (bound_raw is None) validates/redacts in place via _validate_and_redact_candidate(src, target_name) (patch:2343) and then re-reads candidate_body = (src / "SKILL.md").read_text(...) (patch:2363). A writer holding a descriptor opened while the candidate was still at auto/.pending/<slug>/SKILL.md keeps that handle across the claim rename (rename moves the path, not the inode) and can overwrite between the redaction write and the re-read, so unvalidated/unredacted bytes become the live, prompt-injected skill body.

• Conditions are not extreme. auto/.pending is NOT on the agent deny floor — _CREW_SECRET_LEAVES in security/paths.py adds only skills/auto/.private, so an agent file/shell tool can open a pre-claim descriptor to the pending body. The attended path is the DEFAULT (approval_required defaults true).
• This is the exact attack the PR closes elsewhere. The unattended branch reads bound_raw once (patch:2218) and derives the live body from those in-memory bytes precisely because "a writer that opened the candidate's SKILL.md BEFORE the claim keeps a usable handle after the rename" (patch:2209-2214). The attended branch was left re-reading, so the same window stays open on the more common path.
• Recovery: none. Once written, the injected body is live and injected into future contexts; the human approved it believing it was the validated content, and nothing self-corrects.

Cannot complete a rarity/extremeness record — the condition set is the ordinary attended path against an agent-reachable pending directory, i.e. the very LLM-planted-content threat this feature targets. UPHOLD-FENCED.

[ADJUDICATION] 1d24654faf935ba4b5c6cf89418bc3f5319d7baa total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 1d24654faf935ba4b5c6cf89418bc3f5319d7baa

[ADJUDICATION-FENCED] 1d24654faf935ba4b5c6cf89418bc3f5319d7baa fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/skills.py:5269 -- attended path re-reads the claimed SKILL.md after redaction, so a pre-claim held descriptor can swap unvalidated content in before it goes live; pending is agent-reachable and this is the exact race the unattended path closes, so the condition set is not extreme.
[GPT-ADJUDICATED-FENCED] 1d24654faf935ba4b5c6cf89418bc3f5319d7baa

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 1d24654faf935ba4b5c6cf89418bc3f5319d7baa via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Claim-first promotion is the right shape, but fail-closed no-replace support silently wedges the whole review queue, and an unrelated summary-freshness rework rides along.

Watch

• On a filesystem without a native no-replace rename (glibc < 2.28, some NFS/FUSE/overlay data homes), _claim_pending_update refuses every operation — approve, dismiss, dismiss_all_pending, and TTL prune_pending all route through it — so an opted-in user's pending queue accumulates and every review action fails with only a log line ("candidate promotion and dismissal are refused and candidates stay staged for review"). Fail-closed is defensible, but there is no in-product signal telling the operator why, and no remedy short of relocating KIROCREW_HOME.
  Clears when: the probe failure surfaces once through the dashboard/pending API (or a documented operator remedy), instead of per-attempt silent refusals.
• The session-summary freshness change (history.py composite (mtime, size, gen) signature, chat_summary.py, sessions.py, session-summary.md) shares no mechanism with skill promotion; the PR's Problem section motivates only the skills work. Bundling couples revert blast radius: rolling back the promotion machinery also rolls back an unrelated correctness fix, and vice versa.
  Clears when: the summary-signature change moves to its own PR, or a human explicitly accepts the bundling.

[DESIGN-REVIEWED] 1d24654

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 1d24654faf935ba4b5c6cf89418bc3f5319d7baa via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 1d24654

[rubencu]

GPT review disposition — head 811df4f2

BLOCKING (skills.py:3083, linked ancestors) — Accepted / fixed. _private_state_roots_safe now rejects a linked auto namespace root and a linked auto/.pending BEFORE any mkdir, and adds a resolved-containment check (resolved_private.relative_to(self._dir.resolve(strict=True))). The checks are deliberately scoped to the agent-writable segment BELOW the skills dir rather than an unscoped first_linked_ancestor walk: components at/above the skills dir are operator-controlled and legitimately linked on common platforms (macOS /tmp→/private/tmp, ostree /home→/var/home), so a root-first walk would refuse healthy installs. Three regression tests added: linked auto refused with nothing created through the link, linked .pending refused (claim returns None), linked operator prefix still allowed.

Residual (documented in the docstring): these remain path-based checks, so a SAME-PRIVILEGE local writer could in principle swap an ancestor between validation and use. That principal is outside this feature's threat model — the boundary defends against LLM-planted content, and the agent is denied this subtree by the in-process tool gate and the OS sandbox mask; a same-user process that can rewrite the skills dir can rewrite this module instead. Descriptor-relative pinning (mkdirat/renameat on O_NOFOLLOW dir handles) has no portable Windows equivalent and is deliberately not used, consistent with this PR's established documented-residual practice. Defense-in-depth bounds the residual: the guard re-runs at pending.lock setup on the claim path, the containment check rejects an escaped private root, and unattended promotion re-verifies the stage-token candidate binding under the target lock, so swapped content cannot be promoted.

FINDING (skills.py:4538, contradictory comment) — Accepted / fixed. The comment claimed the scripts lexists probe runs before _candidate_layout_ok; the layout guard actually runs first. Rewritten to describe the real rationale (name-probe without following a link; layout already validated no-follow above).

Verification: full changed-surface battery 2,692 tests green; black/flake8 clean; independent GPT + Opus local review lanes both terminal-clean on this exact head.

[rubencu]

Head 4ee367e9 root-causes and fixes the recurring Backend Tests (Windows) (3) failure (test_session_control.py: "too many sessions created recently", seen on f7327c0 and 06a2413):

Root cause — pre-existing test-isolation defect, surfaced by shard repacking. kiro_crew/dashboard/create_rate_limit._buckets is process-wide module state (20 session-creates per 300s window per caller). test_session_control.py drives create_session ~60 times through the real endpoint and never resets the buckets — unlike test_create_rate_limit.py (which resets around every test) and unlike the suite's own _fresh_stop_windows fixture for the analogous stop_retry module state. On slow workers (Windows), creations from this suite plus whatever else shares the xdist worker land inside one 300s window, and a later test's first create reads as the 21st → 429. This PR touches no session-control code — its added test volume shifts xdist shard packing, which is why the failure correlates with this PR and flips between rounds.

Fix: the missing autouse fixture (_fresh_create_buckets calling create_rate_limit.reset_for_tests()), mirroring the file's existing _fresh_stop_windows idiom. Test-only; the product rate-limit window is untouched. Verified: none of the suite's 429 assertions rely on accumulated window state (they exercise the slot-cap paths), full battery 2,834 green.

Remaining external item: the Fork workflow-change guard still needs the allow-fork-workflow-change label (the commit's .github/ changes are the two ratchet-mandated baseline text edits).

[rubencu]

GPT review disposition — head c42972ec

BLOCKING (skills.py:3249, Win32 aliases bypass the reserved-namespace guard) — Accepted / fixed.

DELETE /api/skills/auto. on Windows -> delete_skill resolves auto. as auto -> the entire auto-skill namespace is deleted.

Applied the suggested fix verbatim at the single mutation chokepoint (_live_auto_mutation_lock): the guard now compares namespace.rstrip(" .").casefold() with the reserved namespace, so every Win32 trailing-dot/space alias spelling (auto., auto , auto. ., AUTO.) fails closed before the lock-free manual branch. Verified no false refusal (automation, auto.bar, autobahn pass through — rstrip strips only trailing characters) and that all live-mutation entry points (manual CRUD, auto create/update, pin, injection toggle, archive, restore) route through this chokepoint; slug-component aliases (auto/foo.) were already refused by _promotion_lock's canonical-slug pattern. Regression test extended with the four alias spellings.

FINDING (memory-skills-hooks.md:1463, doc contradicts implementation) — Accepted / fixed.

"native syscall on supported architectures for older glibc" contradicts the implementation

Correct: platform_compat resolves only the libc-exported renameat2 symbol (no raw-syscall fallback). The sentence now states the actual fail-closed behavior: hosts whose libc predates the symbol refuse candidate promotion and dismissal, and candidates stay staged for review.

Verification: extended guard parametrization (8 cases) + full changed-surface battery 2,838 green; black/flake8/docs-lint clean; independent GPT + Opus local review lanes both terminal-clean on this exact head. (Opus noted a pre-existing, out-of-delta follow-up: _probe_no_replace_rename catches OSError but not the NotImplementedError that rename_no_replace raises when the symbol is absent — outcome is unchanged (candidate stays staged) but the refusal is ungraceful; deferring to a follow-up PR rather than widening this one.)

[rubencu]

Design Review dispositions — head c42972ec

Semantic widening of approval_required=false — needs-a-decision (maintainer call), mitigation already shipped.

Operators who set approval_required=false under the old contract upgrade into unattended mutation of live skill bodies … make sure the release names it as a behavior change, or gate updates behind their own sub-flag.

The field's help text was rewritten in this PR to state the new contract explicitly (prose-only updates auto-apply with a rollback snapshot; script-bearing updates ALWAYS stay staged; updates also stay staged when auto_refine_on_deviation is on), and every auto-apply emits the new dashboard notification + WS event. Whether that is sufficient or a dedicated sub-flag (e.g. auto_apply_updates) should gate updates separately is a product contract decision only the maintainer can make — happy to add the sub-flag in this PR or a fast follow-up if preferred.

Scope bundling (session-summary freshness riding along) — needs-a-decision.

Reverting or bisecting either change now drags the other; it deserved its own PR.

Fair critique. The summary-freshness change (sidecar size field + composite signature) has been part of this PR's original scope since it was opened and is independently tested, but it does share no root cause with atomic promotion. Splitting it out now, 18 review rounds in with both blocking lanes green, would re-arm every gate on two PRs; if the maintainer prefers the split regardless, say so and I'll carve it out.

Maintenance surface / invariants live in comments — accepted-and-deferred.

the invariants live in comments rather than a single spec section enumerating them

Agreed. Will add an enumerated invariants section for the claim machinery (lock hierarchy, marker lifecycle, recovery rules) to docs/system-specs/modules/memory-skills-hooks.md in a follow-up, harness-parity style.

Suggestion: treat .promoted marker as legacy-read-only once the lock-state record is authoritative — accepted-and-deferred.
Agreed this would delete the most intricate branch of _cleanup_completed_claim; deferring since it changes crash-recovery semantics and deserves its own review round rather than a rider on round 18.

[rubencu]

First Principles Review dispositions — head c42972ec

Delete SKILL_STATE_ACTIVE/STALE/ARCHIVED (zero consumers) — accepted-and-deferred.
Verified: repo-wide grep confirms the three constants have no consumer beyond their definition. They were added as named anchors for the lifecycle states the archival pass already implements with literals. Removing them is a one-line cleanup I'd fold into any next push of this branch; deliberately not pushing a code change onto a fully-green 65-check board solely for dead constants. If the maintainer wants them gone before merge, one word and the next amend includes it.

Drop the three new kwargs from public approve_pending_update (sole caller passes just slug) — rebutted (keep), with the reasoning on the record.
Verified the call-site claim is accurate: prompts.py:1742 passes only slug, and auto_apply_pending_update calls the private _promote_pending_update directly. The kwargs stay on the public wrapper deliberately: it is a thin signature-preserving shim over _promote_pending_update (same parameter set), so the attended and unattended paths differ only in which arguments they bind — not in which function implements promotion. Hiding the parameters on the private method alone would make the public wrapper's behavior a special case rather than a binding of the same contract. Cost is three defaulted kwargs; benefit is one promotion contract with two bindings.

Item 7 (summary freshness) is a second subsystem — needs-a-decision.
Same as the Design Review's bundling concern: original PR scope, independently tested, but genuinely separable. Maintainer's call whether to split; carving it out re-arms all gates on two PRs this late.

_fresh_create_buckets fixture is an undeclared test-only rider — rebutted.
It is declared and load-bearing for THIS PR: the fixture is the root-cause fix for the twice-recurring Backend Tests (Windows) (3) failure on this PR's own CI runs (rate-limiter module state leaking across xdist-packed tests — full analysis in comment 5506109450). It rides here because this PR's added test volume is what exposed the defect; without it this PR cannot hold a green board.

[rubencu]

CI triage for head f5517d1a: the failing Dependency Audit / Audit Production Dependencies check is not caused by this PR. It flags four newly published fast-uri high advisories (GHSA-5jgf-p345-68v8, GHSA-f65p-4m7j-42xc, GHSA-fph4-wmhf-6fwf, GHSA-jqff-g426-hqxp) in website/electron/package-lock.json — a file this PR does not touch (the audit runs on the merge ref, so it evaluates main's lockfile). This will fail identically for every PR until the dependency is bumped or the advisories are excepted upstream. Holding — no retrigger from this side until the lockfile fix lands on main.

[rubencu]

Follow-up on the audit failure above: the upstream fix landed on main — #7936 (0545b668e, "unpin fast-uri so the patched 3.1.7 can resolve") touches website/electron/package-lock.json. Re-running the failed Dependency Audit / Audit Production Dependencies job should now pass, since the merge ref will pick up the patched lockfile. No changes needed on this PR's side.

[rubencu]

GPT review disposition — head fb4ef0cf

BLOCKING (skills.py ~4566, open candidate handles bypass claimed-snapshot validation) — Accepted / fixed structurally.

Agent retains a writable handle before claim -> rename preserves it -> post-check mutation reaches unattended promotion.

The unattended promotion path now performs exactly ONE read of the candidate body: the bytes verified against the stage-time binding (bound_raw). The promoted live content derives from those bytes in memory (_redact_text(bound_raw) — the identical transform the in-place redactor applies), the shared in-place redactor is not run on this path at all (its independent file read is what made any snapshot-comparison pin non-structural), the claim tree is never mutated, and script copying is skipped entirely under refuse_scripts — so a scripts directory planted after the refusal probe via a retained directory handle can never reach the live tree. After the single verified read there is no file read or write of claim content on the unattended path. Three regression tests pin the interleavings: tamper before the binding read is refused with the candidate restored; tamper after the binding read never reaches the live skill; a post-probe planted script is never copied.

FINDING (skills.py ~4864, pre-claim metadata feeds the notification) — Accepted / fixed.

the notification can describe different bytes than were promoted

The pre-claim _read_pending_meta call is removed from auto_apply_pending_update; the auto-applied notification now carries only values computed from the claimed snapshot under the target lock (name, target, version) and deliberately omits the description, with an in-code comment explaining why.

Attended promotion is behaviorally unchanged. Claim-tree residue semantics are unchanged (pending content was always unredacted at rest; redaction has only ever applied at promotion or display time — verified against source). Verification: changed-surface battery 2,841 green; black/flake8 clean; independent GPT and Opus local review lanes both terminal-clean on this exact head, including the GPT lane that correctly rejected an earlier non-structural pin attempt.

[rubencu]

Disposition — round-21 verdicts on fb4ef0cf8 (superseded by fd7767d0f)

BLOCKER: eager private-root creation in SkillsLoader.__init__ (no-new-work-on-gateway-boot-path) — fixed, via synthesis

This finding directly contradicted the round-10 GPT block that demanded the eager creation (the Linux hide loop is isdir-guarded, so a tree created lazily by the first promotion was reachable inside any child spawned before it). Both demands map to real invariants:

• round 10: skills/auto/.private must exist before any sandboxed child can observe it;
• this round: loader construction must not perform synchronous filesystem work on the gateway boot path.

Loader-init was only ever a proxy for the first invariant. fd7767d0f re-anchors the guarantee at the point it actually protects — spawn time:

• skills.py — the eager-init hunk is fully removed; __init__ performs zero private-root filesystem work (locked in by test_loader_init_does_no_private_root_filesystem_work).
• sandbox.py — the Linux launcher template pre-creates skills/auto/.private in the child's own setup, immediately before the isdir-guarded hide loop. Scoped to that single leaf; the agent-writable auto component is refused when linked (without following it); an absent skills tree is never materialized; any failure degrades to the documented defense-in-depth posture (tool gate + use-time re-authentication). The template is the sole Linux masking mechanism (single builder, single caller), so every masked child runs the pre-create — a spawn that bypasses the launcher has no hide loop and was never protected by init-time creation either.
• Residual is strictly narrower than the init placement: the old residual spanned every child spawned between gateway boot and first promotion; the new one is a single child's own failed mkdir. macOS needs no equivalent (the seatbelt denies the leaf by subpath, unconditionally, with no existence gating); Windows has no OS mask (tool-gate only).
• New tests: launcher block present before the hide loop in every mode; functional twin covering create / linked-auto skip / absent-tree skip.

Both rounds' literal asks are now simultaneously satisfied by one mechanism. If this synthesis is re-blocked, we will escalate to the maintainer rather than iterate further on this subsystem.

FINDING: .pending described as a component between self._dir and the private roots — fixed

Comment now describes auto as the sole intermediate component and auto/.pending as a sibling source root checked separately (the claim rename reads candidates out of it).

FINDING: "immutable snapshot" wording in _claim_pending_update docstring — fixed

Now reads "private claimed snapshot" — the claim is private and uniquely named, not immutable against a pre-claim retained handle; that class is handled by the single-read promotion (bound_raw) shipped in fb4ef0cf8.

Verification on fd7767d0f: full local battery green (2,845 across the affected suites and neighbors), black/flake8 clean, delta-scoped dual review (GPT + Opus lanes) clean on the exact head.

[rubencu]

Head moved fd7767d0f → 5a7f84758 for two reasons, no review findings involved:

1. Windows CI red (own test): test_launcher_precreate_logic_creates_missing_root built paths with pathlib, so Windows separators correctly failed the launcher block's endswith("skills/auto/.private") guard. The test is now @_POSIX_ONLY like its sibling — the block it mirrors only ever runs inside the Linux namespace launcher, whose SENSITIVE_DIRS entries always carry / separators.
2. Merge conflict repair: rebased onto current main (101 commits). dashboard/handlers/prompts.py is ceded to main entirely — main independently landed the executor-offload for the same three skill-mutation call sites this PR had patched, so the file dropped out of the diff (25 → 24 files). In skills.py, update_skill now composes both sides' invariants: this PR's _live_auto_mutation_lock wrapper delegates to _update_skill_unlocked, whose body is main's new pinned descriptor-relative write path, unmodified. Lock ordering verified deadlock-free against the claim machinery's hierarchy (live path: target-lock → pin fd; promotion: claim → target; no inverted pair).

Full local battery (2,925) green on the new head; delta-scoped GPT + Opus lanes both clean on the exact SHA.

[rubencu]

Disposition — stage-2 GPT verdicts on 5a7f84758 (superseded by a38c13efd)

BLOCKING (skills.py:4012): binding computed from a re-read of the public candidate — fixed

Validated real. _stage_pending_skill computed the unattended binding from (pdir / "SKILL.md").read_bytes() after writing — and the pending root is publicly writable, so an overwrite landing in that window would be vouched for, and the unattended promotion trusts the binding. Fixed exactly as asked: the content is encoded once (content_bytes = content.encode("utf-8")), written via write_bytes(content_bytes), and that same byte object is passed to _auto_apply_candidate_binding — zero read-back. This closes the staging-side twin of the round-20 retained-handle fix (promotion side already reads once from the claimed path): the full stage→claim→verify→promote chain now never trusts publicly-writable bytes without either the binding check or claim isolation covering them.

New regression test test_staging_binds_validated_bytes_not_file_reread: a concurrent overwrite injected inside the staging window (via the .meta.json write hook) lands on disk but is refused at promotion — the binding vouches only for the validated bytes, live is untouched. test_auto_apply_binding_uses_exact_staged_bytes was rewritten: its old premise (a text-mode newline-translation layer between validated content and disk) no longer exists under the raw-bytes write; _build_auto_skill_content normalizes \r\n → \n before staging, so the on-disk contract is LF-only and the test now locks that plus untampered promotion.

BLOCKING (skills.py:4034): reads/probes before inode validation in _read_candidate_meta / emit_pending_staged — rebutted

Both functions are byte-identical to head c42972ec9, which this same lane reviewed with "no blocking findings" (the readiness: passed head; the only nearby change since is a one-word docstring fix this lane itself requested). The link/junction probes precede every traversing read in both functions, exactly as they did then. Same-privilege writers planting special files in the pending root are the documented accepted residual (see the _private_state_roots_safe docstring and disposition 5505177043): the pending queue is unattended-input by design, the in-process tool gate and OS sandbox mask fence the agent paths, and the claim/lock machinery re-authenticates at use time. Re-raising this class on unchanged, previously-passed code is the oscillation pattern documented in dispositions 5505177043 and 5519513610; per that precedent it is rebutted rather than patched.

Verification on a38c13efd: battery 2,926 green, black/flake8 clean, delta-scoped GPT + Opus lanes clean on the exact head (Opus additionally traced the end-to-end binding chain and the CRLF contract inversion).

[rubencu]

Disposition — advisory FINDING on a38c13efd (verdict: no blocking findings)

• platform_compat.py:724 — rename_noreplace lacks a native-syscall fallback on old-glibc Linux — accepted-and-deferred.

  On supported old-glibc Linux, rename_noreplace(...) lacks the promised syscall fallback, so all pending-skill approvals and dismissals are refused

  Acknowledged and deliberately fail-closed. os.rename_noreplace-equivalent semantics require RENAME_NOREPLACE (glibc ≥ 2.28 exposes renameat2); on older glibc the helper refuses rather than silently downgrading to a replace-capable rename, because a silent downgrade would reopen the exact clobber class this PR closes. The docs were corrected in an earlier round to promise exactly this fail-closed behavior (no phantom syscall fallback is claimed). A raw-syscall fallback (syscall(SYS_renameat2, ...) via ctypes) is feasible but is hardening for EOL-adjacent platforms, orthogonal to this PR's mechanism, and touches a shared platform shim other subsystems use — it belongs in its own reviewed change. Deferred as a follow-up rather than widening this diff; the practical impact is confined to glibc < 2.28 hosts, where pending-skill approval degrades loudly (refusal with a logged reason), never unsafely.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #7617 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7617: MERGE_DISCUSSION. No code conflict, but the two open PRs encode opposite answers to the same question — whether a candidate that supplied only invalid scripts must remain inspectable. One rule should be settled before either lands. Files: src/kiro_crew/history_consolidation.py, src/kiro_crew/dashboard/handlers/core.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

@rubencu This PR and #7617 (also yours) both change src/kiro_crew/history_consolidation.py in HistoryConsolidator._process_auto_skills, and they answer one policy question differently.

What overlaps: #7617 rewrites the NEW-candidate branch (if self._approval_required or valid_scripts or scripts_supplied:) so a script-bearing candidate whose scripts are all invalid is dropped when approval is off, and it hot-reloads skills.approval_required in api_kirocrew_config_patch in src/kiro_crew/dashboard/handlers/core.py. This PR leaves that branch untouched and extends the UPDATE branch instead: _stage_skill_update gains scripts_supplied and allow_auto_apply, plus a new _auto_apply_staged_update that calls loader.auto_apply_pending_update.

What differs: with approval off, #7617 discards a script-supplying candidate, while this PR keeps a script-supplying update staged for human review. Same question, opposite answers. There is no textual conflict today, but this PR's call site at @@ -1750,6 +1843,8 @@ sits about 90 lines from #7617's hunk, so a mechanical rebase conflict is likely.

Which side is further along: #7617 touches 4 files. This PR touches 24 files, carries two blocking CHANGES_REQUESTED reviews, and is dirty at 635 commits behind, since main split src/kiro_crew/security.py into src/kiro_crew/security/paths.py.

Suggested sequence: land #7617 first, write the single rule for approval_required=false with script-bearing candidates into docs/system-specs/modules/memory-skills-hooks.md, then rebase this PR on top and make the UPDATE path follow that same rule. Both PRs are yours, so the ordering is your call.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

[bolichen97]

Rebased onto main 9af9543b by a maintainer as part of the 2026-09-08 open-PR audit (was 635 commits behind, mergeable_state: dirty). Old head a38c13ef -> new head 1d24654f; diff unchanged at 24 files, +3766/-381.

Conflicts resolved:

• src/kiro_crew/security.py (modify/delete): main split it into a package (refactor(security): split security.py into a package and drop path regex #9183/refactor(security): drop the regex traversal simulation from the bash gate #9089), so the _CREW_SECRET_LEAVES entry for skills/auto/.private moved verbatim to src/kiro_crew/security/paths.py.
• src/kiro_crew/dashboard/server.py: kept main new stall_attribution import alongside your multi-line kiro_crew.skills import with set_update_auto_applied_hook.
• test/test_sandbox_governance_mask.py: end-of-file append from both sides; kept main new test classes and your two launcher pre-create tests.

Gates run locally on changed files: isort, flake8, black --check (the 3 flagged files are in .github/black-baseline.txt), and pytest over the 10 touched test files plus test_skills.py / test_config_baseline.py (567 passed, 25 skipped).

Please review the resolution. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo last-push rule. Reply if anything looks wrong.