feat(skills): auto-apply prose-only skill updates when approval off

[rubencu]

Problem

skills.approval_required=false is documented as letting prose-only auto-skill candidates go live without review — but that only ever applied to new candidates. UPDATE proposals against already-live auto-skills were staged unconditionally: the update branch in HistoryConsolidator called _stage_skill_update without ever consulting the flag.

The result is an incoherent asymmetry:

• A brand-new prose skill goes live immediately with the flag off, yet a prose-only refinement of that same skill sits in the pending queue forever.
• Updates are actually the safer of the two operations: approve_pending_update snapshots the current live SKILL.md to auto/<target>/.versions/v<N>-SKILL.md before overwriting, so every applied update is restorable — new candidates have no such rollback.
• On instances that opted out of review, update candidates rot in auto/.pending/ for days: the user disabled the queue, so nobody is reading it.

Change

When approval_required is false and an update candidate is prose-only, the staged candidate is now immediately promoted through the existing approve_pending_update path — no parallel write path. All of its guards apply unchanged: base-version staleness refusal, symlink/layout guards, redaction, live version snapshot, MAX_SKILL_VERSIONS pruning, and audit logging.

Details:

• SkillsLoader.auto_apply_pending_update(slug) — thin wrapper over approve_pending_update that refuses script-bearing candidates (checks both the .meta.json flag and the physical scripts/ dir) and emits an informational "auto-updated to vN" notification on success.
• HistoryConsolidator._stage_skill_update decides eligibility: flag off and the candidate supplied no scripts at all. A candidate that supplied scripts never auto-applies, even if the validator rejected every script — mirroring the existing new-candidate rule.
• Notifications stay truthful: staging suppresses the "awaiting review" notification only when promotion is about to happen (stage_skill_candidate(notify=False)); if promotion then fails, the review request is re-fired via emit_pending_staged so the still-pending candidate never sits invisible. On success an informational notification ("skill auto-updated to vN", prior version restorable) is raised instead — new set_update_auto_applied_hook, wired in the gateway next to the existing pending-staged hook.
• Audit: successful promotions log an auto_applied_update outcome alongside the existing staged_update event.
• An identity check guards the slug-collision deferral edge in stage_skill_candidate (which can return a name without staging when the collision family is exhausted) so auto-apply can never promote a different, previously-staged candidate.

Fail-safe: any refusal or error during promotion leaves the candidate staged in the pending queue exactly as before.

Unchanged: script-bearing updates always stage; approval_required=true always stages (default behavior untouched).

Also fixes the config documentation gap: the skills.approval_required description previously mentioned the script exception but said nothing about updates. It now states the full semantics.

Tests

12 new tests (plus 7 unit tests on the routing logic in the existing update-flow module):

• prose-only update auto-applies with the flag off — live version increments, .versions/ snapshot exists, queue empty, informational notification fired, review request not fired
• script-bearing update still stages with the flag off
• prose update still stages with the flag on
• promotion failure (refusal or exception) leaves the candidate staged and re-fires the review notification
• a candidate that supplied only rejected scripts still stages
• loader-level guards: script refusal via meta flag, script refusal via physical scripts/ dir (defense in depth against direct-write candidates with missing metadata), stale-base refusal inherited from approve_pending_update
• notify=False suppression + emit_pending_staged re-fire payload contract

Verified locally: targeted skill test modules (118 passed), isort / flake8 clean, mypy clean (877 files).

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Advisory design-level review of 56f44ab23c2c998f5cc104b0f2ac6fa633c36a3b via the fork AI-review pipeline — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

Sound shape — promotes through the existing approval chokepoint — but two real behaviors shipped undocumented: the refine veto and a new lock on the human path.

Watch

• The description claims the config text "now states the full semantics," but the diff's eligibility is allow_auto_apply=not self._auto_refine_enabled: on any instance with auto_refine_on_deviation=true, prose updates never auto-apply — silently recreating the exact queue-rot this PR fixes for those users. Neither the config description hunk nor the memory-skills-hooks.md hunk mentions the refine exception; state it in both (the rationale in the history.py docstring is good — surface it to users).
• The per-target flock promotion lock (auto/.locks/, 10s bounded refusal) is nowhere in the PR description and now gates the pre-existing human dashboard approval path too — a lock-timeout turns an operator's approve into a silent None. It's a justified fix for a real last-write-wins race, but it's an undescribed behavior change on an existing surface: call it out in the description and add the .locks/ layout to the module spec in this commit (spec-same-commit rule).

Suggestions

• The refine veto is config-scoped because update_auto_skill bypasses the new lock; noting that as the named unlock condition (in the spec, not just the docstring) keeps the veto from ossifying into permanent behavior.

[DESIGN-REVIEWED] 56f44ab

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

I've verified the key facts. The candidate concerns the notification body composed at server.py:2213-2224 from an LLM-authored description, rendered as markdown by the frontend.

The candidate's claimed outcome (c) — markdown/HTML injection — does not hold:

1. The frontend renders n.body via MarkdownRenderer (NotificationDetailPanel.tsx:110), whose rehypeSanitize is an allowlist sanitizer: non-allowlisted tags collapse to escaped text, and javascript:/data:/vbscript: URL protocols are dropped (DANGEROUS_PROTOCOLS). So [x](javascript:...) and script/iframe payloads produce no execution — there is no XSS.
2. Rendering a description as markdown (bold, plain links) in the same OS user's own notification feed is the deliberate design, identical to the pre-existing staged-skill notification at server.py:2099-2101. The trust boundary here is the single OS user.
3. The description is additionally credential/URL-redacted upstream.

There is no observable wrong outcome; the item is benign-by-design and its own confidence line rates it low. It fails criterion (c) and is dropped.

I traced the rest of the diff (stage-token ownership check, the deferred-re-stage / notify-suppression / re-fire logic, the per-target promotion lock, the refuse_scripts TOCTOU closure, and the event-loop marshalling) and found no concrete high-confidence defect on the added lines that survives falsification.

No findings.

[OPUS-REVIEWED] 56f44ab

[github-actions]

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

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

BLOCKING -- src/kiro_crew/skills.py:3427 -- Late script injection deletes an unreviewed candidate
if not refuse_scripts and src_scripts.is_dir():
Concurrent direct writer adds scripts/ after the preceding check -> copy is skipped and shutil.rmtree(src) runs -> script-bearing candidate is deleted while prose goes live.
Anchor: residual/crash-data-loss-corruption
Fix: Revert unattended update promotion until candidate contents can be claimed atomically.
FINDING -- src/kiro_crew/history.py:5724 -- "allow_auto_apply=not self._auto_refine_enabled" leaves prose updates pending when both settings are enabled, contradicting the changed documentation and stated approval-off behavior -> Fix: remove the veto or document this exception in the changed configuration/spec text.
[BLOCK-MERGE] 56f44ab
[GPT-REVIEWED] 56f44ab

[rubencu]

Disposition: GPT review of 1557935 — all three findings fixed in 46f1c66

Same-target promotion is now serialized with a per-target cross-process file lock: flock(LOCK_EX) on auto/.locks/<target-slug>.lock (via the existing flock_compat shim), acquired in approve_pending_update around the entire version-check → refuse_scripts re-check → live copy → candidate-delete sequence. auto_apply_pending_update delegates through the same path, so human approvals and unattended auto-applies serialize against each other too. Acquisition is a bounded non-blocking poll (10s), never a blocking wait — on timeout the promotion is refused and the candidate stays pending. A crashed holder cannot deadlock promotion: flock is released by the kernel on process death.

1. skills.py — late refuse_scripts TOCTOU (BLOCKING)
The re-check now runs inside the lock, and the script-copy block is additionally guarded with not refuse_scripts — under unattended promotion the copy path is statically unreachable, so a scripts/ dir injected at any point after the check can never go live. Test test_script_injection_after_lock_acquisition_refused injects a script dir mid-promotion, proves (with a second-fd non-blocking flock probe) that the per-target lock is held at that instant, and asserts the promotion aborts with nothing copied.

2. history.py — concurrent auto-applies last-write-win (BLOCKING)
Both racing promoters now go through the per-target lock. The second promoter re-reads the live version under the lock after the first commits, and is refused by the existing stale-base check — its candidate stays pending for review instead of being deleted and audited successful. Test test_concurrent_same_target_promotions_serialize runs two loaders in two threads (real flock, separate fds) promoting to the same target from the same base: exactly one version bump, the loser refused, the loser's candidate preserved, and the loser's body verified absent from the live skill. test_promotion_refused_while_lock_held_elsewhere covers the fail-safe timeout path.

3. history.py — suppressed staging notification on get_pending_skill failure
The except Exception path now distinguishes a transient lookup failure from a genuine ownership mismatch: it audits pending_lookup_failed and calls loader.emit_pending_staged(staged_slug) so the review request that staging suppressed is re-fired and the candidate is not left invisible. Test test_pending_lookup_failure_refires_staged_notification asserts the notification fires exactly once for the staged slug and the distinct audit reason is recorded.

Verification on 46f1c66: test/test_skill_update_auto_apply.py + test/test_skill_update_flow.py — 54 passed (4 new tests); wider skills/history sweep (46 test files) — 1290 passed, 1 skipped; isort + flake8 clean on the changed files.

[rubencu]

Disposition for GPT round on 79d83bc31 — fixed in 9427e99eb.

Finding accepted: _promotion_lock used flock_compat, which is a no-op on Windows, so concurrent same-base promotions there would not serialize. Switched to platform_compat.try_acquire_lock(fd, exclusive=True) / release_lock(fd) — the repo's cross-platform primitive (POSIX flock, Windows msvcrt.locking region lock, both released on process death). The bounded non-blocking poll and fail-safe refusal semantics are unchanged; the docstring's Windows-no-op caveat is replaced by the cross-platform contract. 18 targeted tests pass (including both serialization tests); isort/flake8 clean.

[rubencu]

Disposition for GPT round on d76344075 — fixed in f8bc802dd.

Finding accepted: the staging flow verified the candidate's stage_token BEFORE the per-target lock, then called auto_apply_pending_update, which promotes whatever candidate occupies the slug at lock time — a concurrent dismiss + same-slug re-stage in that window ships an unreviewed replacement.

Fix (as suggested): stage_token is now threaded through auto_apply_pending_update → approve_pending_update → _approve_pending_update_locked as expected_stage_token, and re-verified against the pending .meta.json INSIDE the lock, right after the locked meta re-read. A mismatch refuses the promotion (audited warning; the replacement stays pending for normal review). The pre-lock check remains as the fast path that also drives the ownership-vs-lookup-failure notification logic; the authoritative identity check now lives under the lock with everything else.

New regression test pins both directions: a swapped candidate with a stale expected token is refused (live version untouched, impostor stays pending), and a matching token still promotes. 19 targeted tests pass; isort/flake8 clean.

[rubencu]

Disposition for GPT round on b58ba77a — fixed in 3fb48b00.

Finding accepted (span 78ebcb5cb0e3, history.py auto-apply): when one consolidation result carried both a new_skill deduped as an UPDATE of target T and a refined_skill for the same T, the update auto-applied first (promoted to live vN+1, candidate deleted), then the refine path overwrote live through update_auto_skill — which takes no version snapshot and whose body the model derived from the pre-update skill — so the just-promoted update was silently destroyed with no rollback copy.

Fix (as suggested — leave the update staged): _process_auto_skills now checks, before staging the UPDATE, whether the same result also names the update's target in refined_skill (gated on auto-refine being enabled, since with it off the refine path never runs). If so it passes allow_auto_apply=False to _stage_skill_update, which vetoes the promotion: the candidate stays staged with its awaiting-review notification fired, and is reviewed against the refined skill. A refine of a different target does not veto — the prose-only auto-apply behavior is unchanged there.

Two new regression tests: test_update_stays_staged_when_same_result_refines_target drives _process_auto_skills end-to-end with both keys on one result and asserts the update candidate survives in the pending queue (body intact), no auto_applied_update audit fires, and the refined body owns live — it fails without the fix exactly on the loss (pending queue empty). test_refine_of_other_target_does_not_veto_auto_apply pins the veto's scope.

Verification on 3fb48b00: skills/history sweep — 417 passed; targeted update-flow files — 57 passed; isort + flake8 clean; mypy clean on history.py.

[github-actions]

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

Advisory premise-level review of 56f44ab23c2c998f5cc104b0f2ac6fa633c36a3b 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; does not block merge.

All evidence gathered. I verified the base tree: the update-staging branch (history.py:5751-5761) indeed never consulted _approval_required; stage_skill_candidate has exactly 2 callers (history.py:5604, history.py:5773); the refine writer update_auto_skill (history.py:5902) writes live unlocked; the dashboard approve handler (prompts.py:451) is the only pre-existing promoter. Final review follows.

First-Principles-Verdict: CONCERNS

The cross-process promotion lock and the refine-mode veto ship undeclared — and the veto contradicts the unconditional auto-apply semantics this PR writes into the docs.

What this change ships

Intent: let prose-only skill updates go live without review on instances that disabled the approval queue — a FIX for an asymmetry in an existing flag's semantics.

1. Prose-only updates go live immediately when approval is off — justified
2. Auto-apply silently disabled whenever auto_refine_on_deviation is on — undeclared, contradicts the docs added here
3. Human dashboard update-approvals now serialize under a per-target flock, refusable after a 10s poll — undeclared
4. New persisted auto/.locks/ dir of never-deleted lock files — undeclared
5. "Skill auto-updated to vN" notification + set_update_auto_applied_hook — justified
6. Review notification suppressed when promotion is imminent, re-fired on failure — justified
7. New SEL outcomes auto_applied_update / auto_apply_failed — justified
8. stage_token field persisted in .meta.json — symptom-level
9. Script refusal enforced through the whole promotion (mid-flight injection aborts) — justified (review choke-point boundary)
10. Config/doc text states update semantics — omits the refine exception

Watch

• Description says "Unchanged: … default behavior untouched," yet approve_pending_update — the human path — now acquires a lock and returns None on a 10s timeout. Real fix, wrong declaration.
• The doc hunk ("this covers both new candidates and prose-only updates") is contradicted by allow_auto_apply=not self._auto_refine_enabled: with refine on, updates still rot in the queue — the exact harm the PR names — and neither doc nor config text says so.
• stage_token patches around the cause: stage_skill_candidate returns a name it did not stage on deferred re-stage. 2 callers total (history.py:5604, history.py:5773); a truthful return (None/sentinel on deferral) deletes the token, the meta field, the pre-lock lookup, and the ownership_mismatch branch.

Subtractions

• Drop the allow_auto_apply parameter — 1 caller, always passing not self._auto_refine_enabled, an attribute _stage_skill_update already reads off self.
• Replace the stage_token apparatus with a truthful stage_skill_candidate return contract (2 callers counted); keep only the in-lock re-check if the dismiss/re-stage window is deemed real.

[FIRST-PRINCIPLES-REVIEWED] 56f44ab

[rubencu]

Round 9 BLOCKING (history.py — concurrent refinement can overwrite an auto-applied update): fixed in 56f44ab.

Applied the reviewer's literal fix: auto-apply is now disabled whenever auto-refine is enabled (allow_auto_apply=not self._auto_refine_enabled), replacing the round-8 result-scoped veto. Routing update_auto_skill() through the per-target promotion lock was considered and rejected: the refine write leaves the version frontmatter unchanged and takes no snapshot, so even a promotion serialized behind a refine passes the stale-base check and the two writers still last-write-win — closing that would mean making the refine path version-aware, a redesign of the promotion subsystem. Disabling auto-apply under refine removes the unattended writer from the race entirely and fails safe: the update candidate stays staged and is reviewed against whatever body the refine left behind. Added a regression test that interleaves a concurrent session's refine into the stage-then-promote window (it reproduces the silent data loss on the pre-fix code) plus an updated test asserting the guard is config-scoped rather than result-scoped. Verified: 59 tests in the auto-apply/update-flow suites, 203 in the skills pending/versioning/lifecycle suites, isort/flake8/mypy clean on touched files.

[rubencu]

Closing per the escalation rule after GPT round 10 on the promotion subsystem (rounds 6-10 all found real check-to-use interleavings in non-atomic candidate consumption; the latest: scripts/ injected between the refuse_scripts check and rmtree deletes an unreviewed candidate while its prose goes live). The convergent fix is structural — claim the candidate atomically via rename before inspecting it — and is written up with the full design history in #3795. Branch preserved at 56f44ab with 262 targeted tests green; the per-target lock and stage-token work remain valid on top of the atomic claim.

[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. Recorded as evidence that the invariant has been reaffirmed more than once in this repo; it imposes no requirement on 7617 by itself. Files: src/kiro_crew/history_consolidation.py.

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