feat(skills): stage similar auto-skills as versioned pending updates + notify on review

[iamwhatever]

Problem

When auto-skill generation produced a candidate that overlapped an existing
generated skill, the candidate was dropped outright (SEL outcome
similar_exists). A later session that learned a genuinely new requirement for a
skill you already have — an extra step, a new failure mode, a rollback
procedure — silently threw that learning away. The skill stayed frozen at
whatever the first session happened to capture.

Separately, a candidate staged for approval was invisible until you happened to
open Capabilities → Skills. Nothing told you anything was waiting, so candidates
accumulated unreviewed.

Why it matters

Auto-skill generation only pays off if skills improve as the agent learns.
Dropping every near-duplicate capped each skill at its first version and quietly
discarded work the agent had already done. And a human-approval gate that never
notifies the human is a queue nobody drains — the feature's whole value depends on
review actually happening.

Fix (symptom → root cause → change)

Symptom: new requirements for an existing skill vanish.
Root cause: _dedupe_candidate answered a binary question — "is this a
duplicate?" — and the only action for "yes" was to drop. There was no way to
express "same skill, but this adds something".
Change: the dedupe judge now returns a tri-state verdict:

Verdict	Action
NONE	genuinely new → stage as a new candidate (unchanged)
DUP <key>	pure re-detection → drop (unchanged)
UPDATE <key>	same skill + new requirements → stage a pending update

An UPDATE routes through a single background merge turn that folds the new
requirement into the existing body (falling back to the candidate's own procedure
if the merge fails or overflows the size cap), then stages it in the existing
auto/.pending/ queue carrying kind: "update", target, and base_version.
Approving snapshots the current live SKILL.md to auto/<slug>/.versions/v<N>-SKILL.md
and writes the merged content as version N+1, preserving the original
created_at. .versions/ is a dot-directory, so _iter_skill_files prunes it —
historical versions can never trigger, and the live SKILL.md is by construction
the only approved version.

Symptom: pending candidates go unnoticed.
Root cause: nothing was wired to the staging path.
Change: stage_skill_candidate — the single choke point every producer
(consolidation and crystallize) passes through — now fires a module-level
observer hook. The gateway registers one that raises a bell-feed notification
deep-linking to Skills → Pending review and broadcasts skills.pending_changed,
which the frontend uses to invalidate the pending query so an already-open Skills
tab refreshes live.

The hook is module-level rather than a per-instance callback deliberately:
SkillsLoader is constructed ad-hoc in ~10 places (consolidation uses
ContextBuilder.skills; dashboard requests build their own per request), so an
instance callback wired at boot would have missed the consolidation path that
produces most candidates.

Review UI: update candidates render an Update badge and, on Review, a
unified diff of exactly what approving will write — computed server-side with
stdlib difflib in preview_pending_update() and rendered through the existing
DiffBlock, so no JS diff dependency was added. Two edge states are explicit: a
warning when the live skill advanced past the merge's base version, and
dismiss-guidance (with Approve disabled) when the target skill no longer exists.

Tests

• test_skill_dedupe.py (+9) — verdict parsing: NONE, DUP <key>,
  UPDATE <key>, bare-key backward compat, invalid key after a prefix, judge
  exception → fail-open to new, and whole-token matching (a longer key is never
  mis-resolved to a shorter one).
• test_skill_versioning.py (new, 37) — version defaults, staged-update
  metadata, approve happy path (snapshot written, body replaced, version bumped,
  created_at preserved), rejection of a missing target / non-update kind /
  symlinked candidate, failure leaves the candidate intact, prune at the cap,
  .versions/ absent from list_skills(), and that the preview's
  proposed_body is byte-identical to what approve writes.
• test_skill_update_flow.py (new, 26) — verdict→action routing, merge
  fallback paths, frontmatter/fence sanitizing at the merge boundary, and that
  validated scripts survive the UPDATE route.
• test_skill_pending_notify.py (new, 8) — hook fires for new and update
  candidates, reports the scripts flag, is a silent no-op with no hook
  registered, isolates observer exceptions, does not fire when staging is
  rejected, and replaces rather than stacks.
• test_skill_pending_api.py (+7), test_skill_pending.py (+1),
  test_history.py (1 assertion tightened — it had become vacuous under the
  new tuple return, since any tuple is truthy).
• SkillsTabPendingUpdates.test.tsx (new, 6) — badge + target text, diff and
  version label on Review, stale-base warning, orphaned-target guidance with
  Approve disabled, new-candidate parity, and approve routing.

Manual verification

Verified end-to-end against an isolated pod (own port, own KIROCREW_HOME,
live gateway untouched): seeded a live auto/deploy-retry at v2 plus an update
candidate, confirmed the API returned the correct diff and v2 → v3, and confirmed
the live skill appeared in the 29-skill list while the pending update did not —
i.e. the approval gate still holds with updates in play. Also ran the full
lifecycle in an isolated KIROCREW_HOME: stage → hidden from
get_triggered_skills → preview → approve → v1 snapshot on disk → approved
skill live and triggerable → pending queue empty.

The two blocking findings from local review were verified the same way. For the
off-loop notification bug I reproduced the failure directly against the real
DashboardState WS path: calling broadcast_ws from a worker thread left
clients_remaining=0, delivered=0 (every socket evicted), while the marshalled
path gives clients_remaining=1, delivered=1.

One gap stated plainly: the loop-marshalling fix itself has no unit test. The
hook is a closure inside start_dashboard, so testing it would mean restructuring
gateway startup purely for testability — disproportionate for a two-line fix that
is covered by the reproduction above.

Screenshots

Pending update under review — Update badge, Proposed change (v2 → v3), and the
server-computed diff showing the added rollback step and the new trigger:

Narrower viewport (diff column reflow)

Notes

• No config field, packaging entry, or migration is needed. Existing candidates
  without a kind read as "new", so nothing already queued changes behavior.
• Follow-up (not in this PR): docs/system-specs/modules/memory-skills-hooks.md
  and builtin_skills/crystallize/SKILL.md document the pending queue but not
  updates or versioning yet.

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @iamwhatever overrides the GPT 5.6 finding for 54111776392075c3223c8868ffa76b17dda5fe10; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 54111776392075c3223c8868ffa76b17dda5fe10: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of 54111776392075c3223c8868ffa76b17dda5fe10 — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

Sound tri-state design, but the stale-update path quietly re-creates the exact learning-loss this PR fixes — and the UI promises a re-proposal nothing schedules.

Watch

• Stale-base dead end. Two updates can queue against one target (<slug>-update, -2… collision suffixes), both merged from the same base; approving the first makes the second permanently unapprovable — approve_pending_update refuses on base_version != current and the only exit is dismiss. Consolidation has already advanced its message offset, so dismissal discards the learning for good — the very harm the PR's problem statement names. Worse, the UI asserts "dismiss this candidate and a fresh update will be proposed against the current version," but no mechanism re-proposes anything; that happens only if a future session independently re-learns the requirement. Rare in single-user practice, but the refusal path should offer a re-merge against the current base (even just re-running _merge_skill_update on demand) rather than dismiss-only — or the UI copy should stop promising it.

Suggestions

• Drop temp-screenshots/ from the tree — the directory name says temporary, yet every clone ships ~270 KB of PR illustrations; attach images to the PR instead.

[DESIGN-REVIEWED] 5411177

[github-actions]

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging 54111776392075c3223c8868ffa76b17dda5fe10.

Second-order review for 54111776392075c3223c8868ffa76b17dda5fe10; this comment is updated in place on each push.

Review details

Both files read in full — the line-level reviewers reported nothing actionable (Opus 5: no findings; GPT 5.6: human-overridden), so the judgment rests on the Design and UX CONCERNS. Here is my verdict.

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Reasoning on the closest calls:

• Stale-base dead end + overpromising dismiss copy (Design Watch, UX Watch test: validate CI workflows on KiroCrew #1): the concerning path is a refusal path — approve_pending_update fails safe by rejecting a stale candidate rather than silently overwriting a newer live version, and the candidate remains on disk and reviewable until the user explicitly dismisses it. The loss requires a rare double-queued-update race plus a deliberate user dismissal made under misleading copy. That is a UX-expectation defect around an auto-learning convenience feature, not a data-correctness bug the diff triggers in production, and both remedies (reword the copy now; add re-merge-against-current-base later) are fully reversible follow-up changes. No persisted format or API is locked in by deferring.
• Committed temp-screenshots/ (~270 KB): git history is technically permanent, but these are non-sensitive PR illustration PNGs, not secrets — trivial repo bloat, removable from the tree in a follow-up.
• i18n-skipping hardcoded strings and narrow-width title truncation: quality regressions in a new UI panel, both trivially reversible string/CSS changes.

Suggested follow-ups (open as issues — non-blocking)

• Reword the stale-base dismiss copy in SkillsTab.tsx — it promises "a fresh update will be proposed against the current version" but nothing schedules a re-proposal; use the UX reviewer's suggested honest phrasing ("if the requirement still applies, a future session will propose it"). Smallest fix; can land as a one-line PR immediately after merge.
• Offer re-merge instead of dismiss-only on stale-base refusal — when approve_pending_update refuses on base_version != current, let the reviewer re-run _merge_skill_update against the current live base so the queued learning isn't discardable-only. Design work belongs in a follow-up PR to skills.py / prompts.py.
• Internationalize the new Skills-tab strings — "Update" badge, "Adds new requirements to…", "Proposed change (v2 → v3)", and both explanatory panels need pages.overview.skillsTab.* keys; eight non-English locales currently render mixed-language panels.
• Fix the narrow-width pending-update row layout — the candidate title truncates to a single character beside full-width Hide/Approve/Dismiss buttons; let the title wrap or stack the buttons.
• Remove temp-screenshots/ from the tree — attach the PR illustrations to the PR/issue instead of shipping ~270 KB in every clone.
• De-duplicate the row subtitle ("Adds new requirements to X — Adds a rollback step…") — render just the description or To {target}: {description}; the badge already conveys the update kind.

[ARBITER-REVIEWED] 5411177

False positive or not applicable? A repository writer can comment:
/ai-review override arbiter 54111776392075c3223c8868ffa76b17dda5fe10: <one-sentence reason>

For a broader accepted-risk deferral, apply defer-longterm and explain why.

[github-actions]

UX Review (Fable 5) — 🟡 CONCERNS

Advisory UX-level review of 54111776392075c3223c8868ffa76b17dda5fe10 — updated in place on each push; does not block merge.

UX-Verdict: CONCERNS

Solid review flow with a server-true diff, but the stale-update copy promises a re-proposal the system doesn't guarantee, and new strings skip i18n.

Watch

• Stale-base warning overpromises: "dismiss this candidate and a fresh update will be proposed against the current version" (SkillsTab.tsx) — nothing re-proposes automatically; consolidation already advanced past those messages, so the update only reappears if a future session re-teaches it. User dismisses expecting a replacement that may never come and silently loses the learning. Low frequency × moderate impact (silent data loss expectation) — reword to "dismiss this candidate; if the requirement still applies, a future session will propose it against the current version."
• New user-facing strings are hardcoded English amid i18nT siblings: "Update" badge, "Adds new requirements to…", "Proposed change (v2 → v3)", both explanatory panels. Eight non-English locales exist (zh-CN, ru, hi…), so every non-English user sees a mixed-language review panel on every visit. Add pages.overview.skillsTab.* keys.
• The committed pending-update-diff-narrow.png shows the candidate title truncated to a single letter ("a…") beside full-width Hide/Approve/Dismiss buttons — at narrow width the reviewer approves a change whose skill name is unreadable. Let the title wrap or stack the buttons below it.

Suggestions

• Row subtitle "Adds new requirements to auto/deploy-retry — Adds a rollback step…" doubles "Adds"; the Update badge + target already carry the first clause — render To {target}: {description} or just the description.

[UX-REVIEWED] 5411177

[github-actions]

Opus 5 Review — ✅ no blocking findings

Reviewed 54111776392075c3223c8868ffa76b17dda5fe10 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 5411177

Verdict parsed from the review's SHA-scoped output markers for commit 54111776392075c3223c8868ffa76b17dda5fe10.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 54111776392075c3223c8868ffa76b17dda5fe10: <one-sentence reason>

[iamwhatever]

Dispositions for findings raised on 86d42ca545c2a4f3a95ee02c452bbd608a6515f2, now addressed on 6d073905:

• BLOCKING — skills.py script promotion swallowed OSError — fixed. Script promotion in approve_pending_update now uses shutil.copy2 (not move) so the pending candidate stays intact as the rollback source, and any OSError aborts the whole approval: files this run created are removed, the live SKILL.md is restored from the version snapshot, the snapshot is deleted, the redacted candidate is restored, and it returns None. Only files not already present are tracked for rollback, so a script the live skill already shipped is never deleted. Regression test test_approve_update_script_promotion_failure_loses_nothing monkeypatches copy2 to raise and asserts the live body/version are unchanged, no half-promoted script exists, the candidate and its script remain reviewable, and no phantom version snapshot is left; test_approve_update_promotes_scripts_on_success covers the success path incl. the POSIX exec bit.
• FINDING — function-local import of set_pending_staged_hook — fixed. Hoisted to module scope alongside the existing from kiro_crew.skills import SkillsLoader on the same line; the function-local import is removed.

Also fixed this round, from the Opus 5 advisory set:

• _rewrite_update_frontmatter now carries the live skill's pinned flag forward (read via _cached_frontmatter, mirrored in preview_pending_update so the diff equals what approve writes), so approving an update can no longer un-pin a skill and expose it to lifecycle archival.
• _stage_skill_update returns early (audited, reason: target_not_live) when the target has no live body, so an UPDATE auto/<pending-slug> verdict can no longer queue a candidate that approve_pending_update would reject forever.

[iamwhatever]

Disposition for the finding raised on 6d0739054c2dbed34506b1f1cb3a2971ee9bf32d, addressed on b6e47e23:

• BLOCKING — rollback left an overwritten live script replaced — fixed. This was a real gap in the previous round's own rollback: it tracked only files the promotion created, so replacing an existing live script and then failing on a later file rolled SKILL.md back while leaving the replacement script live (an internally inconsistent skill). Promotion now snapshots each pre-existing destination's bytes and mode before overwriting it, and the rollback restores those alongside deleting the newly-created files. The snapshot is taken before the overwrite, so a read failure aborts rather than clobbering content it could not restore. Regression test test_approve_update_rollback_restores_overwritten_live_script stages two scripts (one overwriting an existing live script, one that fails), then asserts the overwritten script is byte- and mode-identical to the original, the newly-created script is gone, SKILL.md and the version are unchanged, and the candidate is still reviewable.

[iamwhatever]

Dispositions for the findings raised on b6e47e2312e79dd638ff189701b765fbc3553880, addressed on 6ae05f08 (also rebased onto latest main):

• BLOCKING — refinement can overwrite an existing version snapshot — fixed, at the root cause plus a fail-safe. Confirmed: _build_auto_skill_content emits no version (or pinned) line, so update_auto_skill — the auto-refine path — stripped both; the live skill then read as v1 and the next update-approval would snapshot over the original .versions/v1-SKILL.md. Two changes: (1) update_auto_skill now re-emits version and pinned from the existing frontmatter, so refinement no longer drops them (this also closes the same un-pin hole on the refine path that was fixed for the approve path earlier); (2) new _resolve_snapshot_version — if a snapshot already exists at the frontmatter's version, numbering has drifted, so the snapshot continues above the highest existing number instead of clobbering it, with a warning. History is now append-only. Tests: test_refine_preserves_version_and_pinned and test_approve_update_never_clobbers_an_existing_snapshot (pre-seeds a v1 snapshot with a sentinel, approves, and asserts the sentinel survives while the body lands at v2).
• BLOCKING — script promotion follows symlinked live directories — fixed. The candidate side was symlink-guarded but the LIVE side was not, and unlike approve_pending_skill (which moves into a fresh dest) this path writes into the existing live dir, so a symlinked scripts/ would let mkdir/copy2 follow the link and write outside the skill directory. approve_pending_update now runs _candidate_has_symlink(live_dir) and refuses before any mutation. Test: test_approve_update_rejects_symlinked_live_scripts_dir symlinks live scripts/ to a temp dir, approves, and asserts nothing was written outside, live is unchanged, and the candidate is still reviewable (skipped on Windows, where symlink creation needs privileges).

[iamwhatever]

Dispositions for the findings raised on 6ae05f0853285ea360df20fff479d27d20ebda6b, addressed on db3ec52c (also rebased onto latest main):

• BLOCKING — live skill reads can follow a swapped symlink into credential storage — fixed. Confirmed the exposure is real and worse than a read: read_auto_skill_body guarded the slug and .exists() but never the symlink, and its result is fed to the update-merge turn unredacted (redaction runs on the merge output), so a swapped SKILL.md symlink would put credential bytes into an LLM prompt. The reader now rejects a symlink on either the skill directory or the file, resolves the real path, and refuses it if it escapes the skills tree or hits is_sensitive_path. preview_pending_update was reading the live file directly with the same gap — it now routes through the guarded reader, so the dashboard API inherits the check. Tests: test_read_auto_skill_body_refuses_symlinked_skill_file, ..._symlinked_skill_dir, test_preview_pending_update_refuses_symlinked_live_body (each symlinks to a fake credentials file and asserts None), plus test_read_auto_skill_body_still_reads_a_normal_skill so the guard cannot silently break the ordinary path.
• BLOCKING — valid long skill slugs cannot stage updates — fixed. Verified empirically: _AUTO_NAME_PATTERN caps at 64 chars and the generation prompt permits up to 60, so a 58–64 char target produced a <slug>-update that staging rejected — dropping the learning permanently, because consolidation advances its message offset regardless of candidate outcome. The staged slug is now target_slug[:54].rstrip('-') + "-update", reserving 7 chars for the suffix and 3 for the -2…-50 collision suffix; the full target is still recorded so approval resolves correctly. The rejection-path SEL metadata now reports the slug actually attempted. Test: test_stage_update_truncates_long_target_slug asserts the naive suffix is illegal, the staged slug is legal both alone and with a -50 collision suffix, and the untruncated target survives in metadata.

[iamwhatever]

Dispositions for the findings raised on 3cfaa726bf5a5c7327385f45ceab9e7767070b7d, now on 0ba58c97:

• BLOCKING — stale rejection corrupts the pending candidate — fixed. Confirmed, and it is a regression from my own stale-base fix two rounds ago. _validate_and_redact_candidate redacts the candidate IN PLACE and returns the {path: original_bytes} snapshot precisely so the caller can restore on a later failure — its docstring states the contract: "On ANY failure the originals are restored ... so a rejected candidate is never left corrupted." Every other abort path in approve_pending_update honors it (the SKILL.md read failure above the gate, the snapshot failure below it). The staleness gate I added was the only return None that did not.

  The consequence is worse than a leftover temp file, because the stale path deliberately keeps the candidate PENDING so the reviewer can dismiss it — so the candidate stays visible, and the reviewer re-opens a draft whose body has been silently rewritten with redaction placeholders, on a candidate the system reported it had refused to touch. One-line fix: _restore_redacted() before the rejection returns.

  Test: test_stale_rejection_leaves_the_candidate_unredacted stages two v1 updates where the second carries a credential-shaped token, approves the first to advance live to v2, then asserts the second is refused AND its SKILL.md is byte-identical to what was staged. I verified the test fails with the fix reverted and passes with it, so it locks the behavior rather than merely passing alongside it.

• BLOCKING — concurrent approvals can overwrite an accepted update — unchanged: accepted as a known limitation. Fifth round on the same race. Each round restates it against a different line in the same function (get_auto_skill_version here, atomic_write last round, the isinstance check before that) and proposes a different mechanism (file lock here, per-target lock previously), but no new evidence about reachability has been offered. The repo owner's decision stands on the same three grounds: (1) reachability requires one human clicking Approve on two candidates targeting the same skill within the same few milliseconds — unlike every finding in this series that was fixed, all of which were silent loss in ordinary single-user operation; (2) per-target approval locking was explicitly evaluated and reverted as disproportionate on the predecessor PR (feat(skills): staged, opt-in auto-skill generation (lifecycle + dedupe + scripts + crystallize) + Settings toggle #392), where the operative control was settled as the mandatory human approval gate, so adding it here reverses a settled decision rather than making a new one; (3) approve_pending_skill, which predates this PR, is non-atomic in exactly the same way — this is a property of the approval surface generally, not something this PR introduces. Tracked as a follow-up; the repo owner will clear it with a SHA-scoped /ai-review override.

[iamwhatever]

Dispositions for the findings raised on 0ba58c97fb1e22ce3f8c9d521911161391a32aa5, now on 54111776:

• BLOCKING — base version is captured after merging — fixed. Confirmed. In _stage_skill_update, live_body was read at the top, the merge turn ran with a 90s timeout, and only then was base_version sampled — so the recorded base described live after the merge, not the body the merge was computed from. If an approval lands in that window, the candidate carries a v1-derived body stamped base_version: 2, and approve_pending_update's staleness guard — seeing base == current — passes it and overwrites the intervening update. The guard is only as good as the version it is handed, and it was being handed the wrong one.

  Worth separating from the concurrency item below, because the reachability is materially different: this needs one background consolidation staging an update and one ordinary human approval landing inside a ~90-second window. No simultaneity, no two-clicks-in-the-same-millisecond — and consolidation runs unattended, so nobody is watching for it.

  Fix is to capture the version immediately before reading the body it describes. That also fails safe in the right direction: if live advances after the capture, the recorded base is behind, the guard fires, and the candidate is refused rather than silently applied.

  Test: test_base_version_is_captured_before_the_body_it_describes advances live from v1 to v2 from inside the mocked merge turn and asserts the staged base_version is 1, not 2. Verified failing with the fix reverted and passing with it.

• BLOCKING — update approvals are not serialized — unchanged: accepted as a known limitation. Sixth round on the same race. The grounds are unchanged: (1) it requires one human approving two candidates for the same skill within the same few milliseconds, unlike every finding in this series that was fixed — all of which were silent loss in ordinary single-user operation; (2) per-target approval locking was explicitly evaluated and reverted as disproportionate on the predecessor PR (feat(skills): staged, opt-in auto-skill generation (lifecycle + dedupe + scripts + crystallize) + Settings toggle #392), where the operative control was settled as the mandatory human approval gate; (3) approve_pending_skill, which predates this PR, is non-atomic in exactly the same way, so this is a property of the approval surface generally rather than something this PR introduces.

  To be explicit about the relationship between the two findings, since they are adjacent: the ordering bug above is a real defect in the input to the staleness guard and is fixed on its own merits with a two-line move. It is not evidence that the guard must be replaced by locking — the guard, correctly fed, refuses stale writes, and the residual window it does not cover is the millisecond-simultaneity case above. Tracked as a follow-up; the repo owner will clear it with a SHA-scoped /ai-review override.

[iamwhatever]

Disposition for the finding raised on 54111776392075c3223c8868ffa76b17dda5fe10 (no code change; still 54111776):

• BLOCKING — version-only stale check permits overwriting newer edits — rebutted.

  The mechanism is real and I am not disputing it. I verified both writers named: update_auto_skill (auto-refine) deliberately carries the version forward unchanged (version: {_vn}, not _vn + 1), and PUT /api/skills/{name} writes caller-supplied content verbatim via update_skill. So live content can change while version stays put, and a candidate staged at v1 will pass raw_base == current_version. The disagreement is about the remedy, not the mechanism.

  1. The overwrite is visible to the approver, which is the control that matters. preview_pending_update reads the live body at request time, so the diff rendered on the Approve screen is computed against the EDITED content — the reviewer sees their own edited lines as removals in the diff they are approving. This is the specific distinction from the round-5 stale-base finding, which was legitimate and was fixed with a hard reject: there the diff was wrong, because React Query served an already-open sibling candidate's cached v1-based detail, so the reviewer provably could not see the loss. There is no cache indirection on this path (the pending-detail query runs with staleTime: 0 + refetchOnMount: 'always'), so opening the candidate fetches fresh against current live. A human approving a diff that accurately shows the removal is the operative control this feature is built on.

  2. The proposed fingerprint is not merely heavier — it is fragile in a way that trades a visible failure for a silent one. Approval rewrites the candidate's frontmatter (_rewrite_update_frontmatter) and _validate_and_redact_candidate mutates the candidate in place before the gate runs. A base-content fingerprint would therefore need exact normalization rules to avoid drifting for reasons unrelated to any edit; get that wrong and legitimate candidates are silently refused as stale. It also introduces a second source of truth about "has live changed" that can disagree with version, which is the ambiguity the version field exists to remove.

  3. Both no-bump writers are narrow. Auto-refine is gated behind TWO opt-in flags, and the config loader enforces the dependency (auto_refine_on_deviation requires auto_create_from_sessions); both default off. The editor path requires a human to hand-edit a live auto-skill while an update candidate for that same skill is pending — and that human is the same person who then sees the accurate diff and clicks Approve.

  The cheaper follow-up, if we later want version to be a true content marker: bump it on write rather than fingerprinting around it — the guard then catches refine drift with no new stored state and no new failure mode. That is a change to the refine/editor write paths, which are outside this PR's diff, so it is recorded as a follow-up rather than folded in here.

  Repo-owner decision: rely on the version, ship the visible-diff control, and do not add content fingerprinting.

[iamwhatever]

/ai-review override gpt 5411177: The overwrite is not silent — preview_pending_update reads the live body at request time, so the Approve-screen diff shows the newer content being removed (unlike the round-5 stale-base case, which was genuinely invisible via a cached sibling detail and was fixed with a hard reject) — while the proposed base-content fingerprint would have to survive _rewrite_update_frontmatter and in-place redaction and would silently refuse valid candidates when it drifted; both no-bump writers are narrow (auto-refine sits behind two default-off flags whose dependency the config loader enforces, and the editor path is a deliberate human hand-edit by the same person who then approves the accurate diff), and bumping version on write is recorded as the cheaper follow-up instead of adding a second source of truth about whether live changed.

[github-actions]

AI-review override not recorded: keep the reason to 500 characters or fewer.

[iamwhatever]

/ai-review override gpt 5411177: Overwrite is not silent: preview_pending_update reads live at request time, so the Approve diff shows the newer content being removed. A base-content fingerprint must survive the frontmatter rewrite plus in-place redaction, silently refusing valid candidates when it drifts. Both no-bump writers are narrow (auto-refine sits behind two default-off flags; the editor is a deliberate hand-edit by the approver). Bumping version on write is the recorded follow-up.

[github-actions]

Human judgment recorded

@iamwhatever marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 54111776392075c3223c8868ffa76b17dda5fe10.

Overwrite is not silent: preview_pending_update reads live at request time, so the Approve diff shows the newer content being removed. A base-content fingerprint must survive the frontmatter rewrite plus in-place redaction, silently refusing valid candidates when it drifts. Both no-bump writers are narrow (auto-refine sits behind two default-off flags; the editor is a deliberate hand-edit by the approver). Bumping version on write is the recorded follow-up.

This decision applies only to this commit. A new push requires a new judgment.