feat(skills): staged, opt-in auto-skill generation (lifecycle + dedupe + scripts + crystallize) + Settings toggle

[iamwhatever]

Summary

Reworks auto-skill generation into a safe, opt-in, staged pipeline, informed by a code-level review of the mature sibling project (NousResearch/hermes-agent). Generation is off by default — enable it in Settings → Skills (or kirocrew config set skills.auto_create_from_sessions true). When on, nothing goes live unattended — candidates are staged for human approval, deduped, bounded, and (when deterministic) can ship a validated Python script.

Pipeline: detect (during consolidation) → generate → metadata dedupe → pending queue → human approval → live → archive-if-unused.

Phases (each an isolated commit)

• Phase 0 — lifecycle + dedupe primitives. Archive-not-delete lifecycle (active→stale→archived by inactivity) with pin + cron-referenced exemptions and a never-used grace floor; max-N cap as a backstop; archives are recoverable (restore_auto_skill) — the evictor never hard-deletes. Embedding-free metadata dedupe (skills_dedupe.py): one injectable judge call over all generated skills. Dot-dirs (.archive/, .pending/) pruned from discovery. New SkillsConfig fields.
• Phase 1 — staged approval + opt-in default. Pending-queue store (stage/list/get/approve/dismiss/prune; approve marks scripts executable). Consolidator routes new skills to the queue when approval_required (default true); auto_create_from_sessions defaults false (opt-in). Enable via the new Settings → Skills toggle (or CLI). Dashboard API (GET /api/skills/-/pending, .../approve, .../dismiss, POST /api/skills/-/pin) + a self-contained Pending review panel in the Skills tab, and a Settings → Skills panel with the auto-generate + require-approval toggles.
• Phase 2 — Python scripts + always-on validation. Generation may emit scripts[] (Python-only, Windows-portable). Static validator (skills_script_validator.py) blocks destructive commands, credential/sensitive-path access (incl. secret env-var getters), network egress (HTTP clients, sockets, asyncio stream openers, webbrowser), oversize, non-Python, syntax errors — runs even on the auto-approve path, and scripts are re-validated after redaction. Script-bearing candidates always require approval; dangerous scripts are dropped while the prose skill still stages.
• Phase 3 — $crystallize on-demand skill. Builtin skill to capture the current session (incl. [Subagent completion event] output) into a staged candidate, with cross-source dedup.

Design divergences from Hermes (deliberate, KiroCrew-specific)

• Staged + opt-in (Hermes writes live, on by default) — justified by scripts + the dashboard review surface; the operative control is mandatory human approval of every candidate and script.
• Embedding-free metadata dedupe + max-N backstop (Hermes uses LLM umbrella-consolidation + inactivity-only). Adopted from Hermes: archive-not-delete, pin/cron exemptions, grace floor, agentskills.io-style folders, always-on script scanning.

Safety / integrity hardening

• Approve is transactional: symlink + candidate-file allowlist (only SKILL.md, .meta.json, scripts/ dir), re-validation + redaction of the candidate's current bytes, and restore-on-abort on every failure path so a rejected candidate is never left partially-redacted or stranded.
• Pending reads/writes: recursive credential redaction (values and keys) at the detail/list choke, prune_pending ages by filesystem mtime (not LLM-supplied timestamps), set_pinned uses atomic_write, and same-slug staging allocates a unique sibling slug instead of dropping a distinct candidate. skills.py is a registered security_posture redaction sink.

Testing

• Backend: the new suites (test_skill_lifecycle, test_skill_dedupe, test_skill_pending, test_skill_pending_api, test_skill_script_validator, test_crystallize_skill) plus test_security_posture, test_config_baseline, test_history auto-skill classes, and test_config_loader all pass; isort/flake8/mypy clean on changed source.
• Frontend: tsc -b clean; SkillsTab + SkillsPanel.settings vitest pass; npm run build OK.

Config (new / changed defaults)

auto_create_from_sessions → false (opt-in; enable in Settings → Skills), approval_required=true, max_auto_skills=100, stale_after_days=30, archive_after_days=90, pending_ttl_days=30, generate_scripts=true, judge_model=claude-haiku-4.5. Both skills.auto_create_from_sessions and skills.approval_required are on the PATCH /api/config/kirocrew allowlist so the Settings toggle can set them.

Notes / follow-ups

• The Haiku metadata-dedupe judge and advisory pending review are wired as modules/config; the consolidator dedupes via the injectable judge with a lexical find_similar fallback. The judge runs on the shared background lite model (no per-turn model switch).
• Deferred follow-ups: a fully separate forked "generation session"; routing $crystallize through the code-path staging (skill_stage) rather than prose-guided direct writes; and a per-skill pin button in the Skills tab (the pinSkill API + lifecycle exemption already exist).

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @iamwhatever overrides the GPT 5.6 finding for 6ff260d3900043820870770860f19223286f6e08; 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 6ff260d3900043820870770860f19223286f6e08: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound staged pipeline, but the crystallize write path bypasses the repo's MCP-first boundary and the description contradicts the diff on the headline default.

Watch

• Description ↔ diff conflict on the central safety claim. The body says "auto_create_from_sessions now defaults true (staged)"; the code keeps default=False ("Disabled by default; enable in Settings → Skills", loader.py) and the title says "opt-in". A reviewer approving "on-by-default generation" and a reviewer approving "opt-in" are approving different risk postures — state which one shipped.
• Crystallize stages skills by having the LLM hand-write files into auto/.pending/ (SKILL.md step 4) instead of a structured MCP tool, despite AGENTS.md's MCP-First rule for LLM-facing operations. This choice is the root cause of the large compensating surface in skills.py — deep redaction of nested .meta.json keys, symlink walks, top-level entry allowlists, filename redaction — all enforced at read/approve choke points. Every future consumer of the queue must re-remember those checks; one missed choke point leaks unvalidated content. A skill_stage MCP tool would validate/redact once at write time and shrink that surface.
• skills.judge_model is decorative. It defaults to claude-haiku-4.5 and is documented as "model used for the dedupe judge", but _dedupe_judge deliberately runs on the shared kirocrew-lite background session with "no per-turn set_model switch" — the field only gates whether the judge runs. A user setting it to another model changes nothing; ship it as a boolean or make it select the model.

Suggestions

• The archive is the only unbounded store in the design (versioned slug-2, -3… copies, no TTL, while pending gets pending_ttl_days and live gets max_auto_skills); give it the same bounded story.

[DESIGN-REVIEWED] 6ff260d

[github-actions]

Opus 5 Review — ✅ human override accepted

Reviewed 6ff260d3900043820870770860f19223286f6e08 — this comment is updated in place on each push.

Human judgment by @iamwhatever overrides the Opus 5 finding for 6ff260d3900043820870770860f19223286f6e08; the recorded reason is authoritative for this commit.

Verdict recorded from an authorized human decision for commit 6ff260d3900043820870770860f19223286f6e08.

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

[github-actions]

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging 6ff260d3900043820870770860f19223286f6e08.

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

Review details

Arbiter-Verdict: PASS

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

Both line-level reviews (Opus 5, GPT 5.6) carry human overrides recorded as authoritative for this commit, leaving no line-level Medium/Low findings to judge. The design reviewer's three Watch items and one Suggestion were each weighed against the one-way-door and concrete-harm tests; none clears the bar, as detailed below.

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

• PR description ↔ diff conflict on the default (Design Review Watch: "The body says auto_create_from_sessions now defaults true (staged); the code keeps default=False") — the shipped code is the safe posture (opt-in, default=False in loader.py, defaultValue: false in config-baseline.json), so no wrong behavior merges; this is a PR-body correction plus one stale code comment in dashboard/server.py ("contradicting the on-by-default config") that should be fixed to say opt-in. Editable at any time with zero migration; fix the PR description before merge as hygiene and correct the comment in a trivial follow-up.
• Crystallize stages skills via LLM-hand-written files instead of a skill_stage MCP tool (Design Review Watch) — this is the AGENTS.md MCP-first rule and an architectural/maintainability concern, explicitly out of blocking scope. The compensating controls this PR ships (deep .meta.json redaction, symlink rejection, top-level entry allowlist, script re-validation — all at the read/approve choke points in skills.py) mean the immediate risk is mitigated; consolidating validation into a write-time MCP tool is a reversible later refactor. Track as: add skill_stage to mcp_core.py and rewrite the crystallize SKILL.md to call it.
• skills.judge_model is decorative (Design Review Watch: _dedupe_judge runs on the shared kirocrew-lite background session with no per-turn set_model, so the field only gates whether the judge runs) — a misleading config contract, but not locked in: making the field actually select the model later is backward-compatible, and the config schema has a deprecated path if it becomes a boolean instead. No data migration or breaking change is forced by merging. Fix in history.py/config/loader.py (either honor the model or reshape as a boolean with updated help text).
• Archive is the only unbounded store (Design Review Suggestion: versioned slug-2, -3… copies under auto/.archive/, no TTL, while pending gets pending_ttl_days and live gets max_auto_skills) — growth is small text folders accumulating at human review/lifecycle timescale (30–90-day windows, 100-live cap), not a production-crash-class resource leak; a TTL or cap on auto/.archive/ in skills.py::archive_auto_skill/run_skill_lifecycle is a clean later change.

[ARBITER-REVIEWED] 6ff260d

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

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

[iamwhatever]

Addressed all review findings in 513a01c1 (single commit, force-pushed):

GPT 5.6 — HIGH (skills.py: stale scripts on re-stage): stage_skill_candidate now rmtrees any existing pending dir before writing, so a re-staged slug can't retain scripts from a prior candidate.

GPT 5.6 — HIGH (prompts.py: blocking FS on event loop): the pending detail/approve(+lifecycle)/dismiss and pin handlers now run their filesystem work via run_in_executor(discovery_executor(), ...) — no sync FS on the loop, matching no-blocking-call-on-event-loop.

GPT 5.6 — MEDIUM (scripts rendered as a top-level key): the scripts field is now embedded inside the new_skill object shape in the prompt (not a separate numbered key), matching what _process_auto_skills reads (new_skill["scripts"]).

Arbiter — validate at the promotion choke point: approve_pending_skill now re-runs validate_scripts on the candidate's scripts/ and refuses promotion if any script fails — so a crystallize-authored candidate that wrote scripts directly into .pending/ (bypassing stage_skill_candidate) is still scanned before anything is chmod'd executable. The "no unscanned script goes live" invariant is now a property of the pipeline, not a per-producer courtesy.

Arbiter — dead cron-reference import: implemented cron.referenced_skill_names() (reads crons.json, extracts $skill tokens from job messages, best-effort/read-only). The lifecycle exemption now matches either the full key or the bare slug, so a skill a cron job references is genuinely exempt from archival.

Added tests: approve-time re-validation of directly-written pending scripts (test_skill_pending.py), and cron.referenced_skill_names extraction (test_cron_skill_refs.py). Also fixed the flake8 E303/E305 the lint gate caught.

[iamwhatever]

Addressed the second-round findings in 39ef28ba:

HIGH — validator regex denylist is bypassable (__import__("os").remove(...)): added an AST policy pass to validate_skill_script on top of the regex denylist. It rejects dynamic exec/import (eval/exec/compile/__import__), destructive fs calls (os.remove/unlink/rmdir/removedirs, shutil.rmtree, pathlib.*.unlink/rmdir), process execution (os.system/os.popen/subprocess.*), and dynamic import (importlib.import_module, subprocess/ctypes imports). The exact cited payload is now rejected (test added). Runs both at stage time and at the approve_pending_skill choke point.

HIGH — approval UI approved blind: the Pending review panel now has a Review toggle per candidate that fetches the detail endpoint and shows the full SKILL.md body and every bundled script's contents inline before Approve.

MEDIUM — prune_pending had no caller: wired it into api_skills_pending (offloaded via discovery_executor), so listing the queue opportunistically prunes candidates older than pending_ttl_days.

MEDIUM — default-on vs. spec doc: enabling-by-default is an explicit product decision from the PR author; rather than revert it, I updated the governing spec (docs/system-specs/modules/memory-skills-hooks.md) to describe the v2 staged, on-by-default pipeline (staging, approval, scripts, lifecycle, crystallize) and corrected the default in its config example — so docs and runtime agree.

Backend Lint & Type Check (3.10/3.12) is green after the earlier flake8 fixes.

[iamwhatever]

Addressed the round-4 findings:

HIGH — symlink TOCTOU in approve_pending_skill: approval now walks the candidate directory and refuses promotion if any entry is a symlink (os.path.islink), so a script can't be a benign symlink during review/validation and be repointed afterward. Only real files are ever promoted + chmod'd. Test added.

HIGH — _process_auto_skills on the event loop: _consolidate now calls it via await asyncio.to_thread(self._process_auto_skills, result, key), so the synchronous dedupe/stage/lifecycle filesystem work no longer blocks the gateway loop (matches no-blocking-call-on-event-loop).

Also fixed the mypy failure the lint gate caught (set.add() used in a boolean expression in the validator's de-dupe → rewritten as an explicit loop). mypy src/kiro_crew/ and flake8 src/kiro_crew test are both clean locally.

[iamwhatever]

Addressed the round-5 findings:

HIGH — read handlers weren't audited: api_skills_pending (list) and api_skill_pending_detail now emit sel().log_tool_invocation for success / invalid-slug / not-found, matching the backend-security-controls audit rule already followed by approve/dismiss/pin.

MEDIUM — absolute path leak: list_pending_skills() no longer includes the on-disk path field (the API/frontend never used it — the detail endpoint keys on slug), so /api/skills/-/pending no longer exposes the server's home/directory layout.

[iamwhatever]

Addressed in cb3ec9bb:

HIGH — crystallize content reaching dashboard/live unredacted (skills.py): the two-pass redaction (redact_exfiltration_urls + redact_credentials) that _process_auto_skills applies is now also enforced at the two code choke points every producer flows through, so a candidate written directly into the queue by the crystallize skill (or any future producer) cannot surface secrets:

• get_pending_skill (dashboard-facing detail) redacts the SKILL.md body and every script body before returning.
• approve_pending_skill redacts the promoted SKILL.md + all scripts (recursively) before chmod, so no credential/exfil URL goes live.

Chose choke-point redaction over a new skill_stage MCP tool because it covers all producers with no new public surface; the dedicated MCP staging tool is tracked with the already-flagged generation-session follow-up. Regression test test_direct_write_candidate_is_redacted_at_detail_and_approve added.

The prior two HIGHs (nested-script validation recursion, archive-collision preservation) remain fixed.

[iamwhatever]

Addressed in 8e0d8c68:

HIGH — aliased network import bypasses egress detection (skills_script_validator.py): the AST gate now rejects network/egress library imports outright — requests, httpx, aiohttp, urllib, urllib3, http, socket, ftplib, smtplib, telnetlib, poplib, imaplib — matched on the imported module root (not the call site), so alias bypasses like import requests as r; r.get(...) and import socket as s; s.socket() are caught regardless of alias. This matches the skill contract's "must not call unknown network hosts" rule. Regression tests added for aliased import ... as, from urllib import request, and aliased socket.

[iamwhatever]

Addressed in the latest push:

HIGH — re-stage race on the slug-keyed Approve (skills.py stage_skill_candidate): staging no longer clobbers a candidate already awaiting review. A background consolidation that re-detects the same slug is now deferred (returns the existing name, logs, changes nothing) instead of rmtree+rewrite. The queued candidate is immutable from stage until it is approved or dismissed, so the slug-keyed Approve always promotes exactly the bytes the human reviewed — the swap-underneath-review window is eliminated. (A fresh detection for that slug re-stages on a later consolidation once the slot frees.) Regression test test_restage_does_not_clobber_candidate_under_review added.

[iamwhatever]

Addressed in dbb0bbcf:

HIGH — unredacted metadata via pending API (skills.py _read_pending_meta): metadata strings are now redacted (exfiltration URLs + credentials) at the single read choke point feeding both list_pending_skills and get_pending_skill, so a crystallize-written credential can't surface to the dashboard.

HIGH — silent redaction failure on approve (skills.py): _redact_file_in_place now returns success/failure; approval redacts the candidate body + all scripts before the move-to-live and aborts (returns None) if any file can't be read+rewritten — unredacted bytes can no longer reach a live skill.

HIGH — incomplete sensitive-path denylist (skills_script_validator.py): the sensitive-path set is now derived from the canonical security._SENSITIVE_HOME_DIRS (the list backing is_sensitive_path()), so it covers the full credential-dotfile set, the SSO/kiro-cli auth stores, and KiroCrew's governance trust-root files — and stays in sync automatically.

MEDIUM — approve could evict its own skill (prompts.py/skills.py): run_skill_lifecycle gained an exempt set; approval exempts the just-promoted skill from the max-N backstop.

MEDIUM — nested scripts not executable (skills.py): approval now marks scripts executable recursively via os.walk.

MEDIUM — Approve before review (SkillsTab.tsx): the Approve button is disabled until the candidate detail is expanded/loaded.

Regression tests added for each. The remaining pass-2/3 MEDIUMs (autonomous lifecycle trigger, dashboard fallback consolidator wiring, unwired dedupe) are being triaged with the author.

[iamwhatever]

Addressed the remaining MEDIUMs in b9f356e0:

MEDIUM — autonomous lifecycle (history.py): _consolidate now runs run_skill_lifecycle on the existing idle/periodic consolidation path independent of create/approve (throttled to once/hour across sessions), so age-based archival operates on its own instead of only after a skill is created or approved.

MEDIUM — dashboard fallback consolidator (server.py): the auto-created fallback HistoryConsolidator is now wired with the skills loader + full skills config (including judge_model), so a dashboard-only launch honors the on-by-default auto-skill config instead of silently disabling it.

MEDIUM — unwired dedupe judge (history.py + skills_dedupe.py): metadata_dedupe is now the production dedupe path. _process_auto_skills runs a single metadata-judge call over all existing auto-skills, pinned to skills.judge_model on the background session (prior model restored afterward, so consolidation is unaffected); the sync worker bridges the async judge onto the captured loop via run_coroutine_threadsafe. Lexical find_similar remains the fallback when the judge is unavailable (no judge_model, no loop, or no existing skills) — matching the documented behavior. judge_model + generation_model are now consumed (threaded through all five consolidator construction sites). Fail-open throughout.

Tests: _dedupe_candidate judge-bridge + lexical-fallback paths, cap-exemption, nested chmod, meta redaction, expanded sensitive paths. Local gate green (flake8, mypy, 246 skill/history tests, config-baseline).

[iamwhatever]

Addressed in e9958b20:

HIGH — approve-time TOCTOU (skills.py): approve_pending_skill now atomically renames the candidate out of the agent-writable pending tree into a private, dot-prefixed quarantine dir before any symlink check / validation / redaction. Promotion (_promote_snapshot) operates only on that isolated snapshot and the candidate is restored to the pending queue on any rejection. An agent racing to swap a script after review now mutates the vacated pending path, not the promoted snapshot.

MEDIUM — dormant generation_model (config/loader.py): removed the field, its loader parse, the spec config example, and regenerated config-baseline.json. The dedicated generation session that would consume it stays a flagged follow-up; judge_model remains (now wired to the dedupe judge).

MEDIUM — indirect dangerous attribute (skills_script_validator.py): the AST gate now flags a dangerous callable referenced (not just called) off a dangerous module root — f = os.remove; f(x) — scoped to os/shutil/subprocess/importlib/ctypes so benign attributes aren't flagged.

Regression tests: failed-approve restores the candidate to pending; aliased-attribute rejection. Local gate green (flake8, mypy, 229 skill/history/config tests, config-baseline).

[iamwhatever]

Disposition on reviewed SHA 971a8c9d → 7b222988.

• skills.py:1348 — failed .meta.json removal exposes unredacted metadata → FIXED. The except OSError: pass around the pre-promotion .meta.json unlink was too broad: if removal failed (non-writable dir), the raw metadata rode into the live skill dir. Now only FileNotFoundError is benign; any other removal error aborts promotion (return None), leaving the candidate intact in pending. Regression: test_failed_meta_unlink_aborts_promotion.

• skills_script_validator.py:145 — "constructed sensitive-path WRITES bypass validation" → REBUTTED. This is the write-side of the denylist-vs-allowlist point already rebutted for reads. Banning all write_text/rename/replace/write-mode open() is disproportionate — legitimate skill scripts write files — and does not close dynamically-constructed write paths (unbounded). The operative control is mandatory human approval: a script that writes to a governance/sensitive path is exactly what a reviewer rejects, and every script-bearing candidate is queued for review, never auto-published. Literal sensitive paths are already denied; the residual computed-path write is defense-in-depth behind the human gate.

Local gate green (flake8/mypy + skill suites). This round the fail-closed .meta.json guard was a real gap my pre-submit local review missed and the server caught — the intended backstop.

[iamwhatever]

Disposition on reviewed SHA 7b222988 → c5fd32b4. Both fixed.

• skills.py:1337 — redaction can invalidate an already-validated script → FIXED. Scripts are now re-validated after redaction (before the move): the first validate_scripts runs on pre-redaction content, so redacting a credential-shaped token that alters script bytes could previously have promoted a broken/altered helper. If any script is invalid post-redaction, promotion aborts (return None) and the candidate stays in pending. Regression: test_redaction_breaking_script_aborts_promotion.

• prompts.py:490 — pin failures bypass SEL auditing → FIXED. api_skill_pin's set_pinned executor call is now wrapped in try/except — a failure emits log_tool_invocation(outcome='error') and returns a controlled 500, matching the detail/approve/dismiss handlers. Regression: test_pin_executor_failure_audits_and_500s.

The prior write-side validator-denylist finding (skills_script_validator.py:145) is not re-raised on this SHA — the rebuttal (human-approval gate; denylist ≠ allowlist; blanket write-ban is disproportionate) stands. Local gate green (flake8/mypy + skill suites).

[iamwhatever]

/ai-review override gpt aa75d81: mandatory human approval plus approve-time re-validation, redaction, symlink/allowlist checks of the candidate's CURRENT bytes is the operative control; the requested digest-binding + atomic-quarantine machinery was deliberately reverted earlier as disproportionate over-engineering for a human-approval-gated feature, and two independent local mirror reviews (codex-review.yml and claude-review.yml + AUTOSDE contracts) found no blocking issue on this diff.

[github-actions]

Human judgment recorded

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

mandatory human approval plus approve-time re-validation, redaction, symlink/allowlist checks of the candidate's CURRENT bytes is the operative control; the requested digest-binding + atomic-quarantine machinery was deliberately reverted earlier as disproportionate over-engineering for a human-approval-gated feature, and two independent local mirror reviews (codex-review.yml and claude-review.yml + AUTOSDE contracts) found no blocking issue on this diff.

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

[iamwhatever]

Disposition on reviewed SHA ad615486 → bb4a94d5.

• skills.py:1361 — failed promotion destroys pending metadata → FIXED. .meta.json is now stashed (read_bytes) before the pre-move unlink, and shutil.move is wrapped: on a move failure the metadata is restored to the pending dir and approval aborts (return None), so a failed promotion can no longer strand the candidate without its metadata. (The unlink itself remains fail-closed — a non-removable .meta.json still aborts before the move so raw metadata can't ride live.) Regression: test_failed_move_restores_meta.

Also in this push (feature default change): skills.auto_create_from_sessions now defaults OFF (opt-in) — auto-generation is enabled via a new Settings → Skills panel (skills.auto_create_from_sessions + skills.approval_required added to the config PATCH allowlist as plain bools, strictly narrower than the pre-existing PUT /api/config/kirocrew). Config baseline regenerated; frontend tsc + vitest green (SkillsPanel.settings.test.tsx). The prior structural findings (quarantine machinery, blanket fs/env read-ban) remain rebutted — reverted/disproportionate for a human-approval-gated, now opt-in feature.

[iamwhatever]

Disposition on reviewed SHA bb4a94d5 → 4987e6d0.

• skills.py:1337 — failed approval corrupts the pending candidate → FIXED. All redaction targets (SKILL.md + every script) are now snapshotted (read_bytes) BEFORE the in-place redaction loop; any abort after partial redaction — a redact failure OR the post-redaction re-validation failing (e.g. redacting a credential-shaped identifier broke syntax) — restores the original bytes before return None. A failed approval can no longer leave a partially-redacted / syntactically-broken pending draft. Regression: test_redaction_breaking_script_aborts_promotion now asserts the script is restored verbatim.

• skills_script_validator.py:72 — "reject all filesystem-read APIs and their aliases" → REBUTTED (structural, unchanged). This is the fs/env read denylist-vs-allowlist point already rebutted across prior SHAs: banning all file reads (e.g. open/read_text) is disproportionate — legitimate skill scripts read data/config files — and cannot close dynamically-constructed/obfuscated paths generally. Operative control: mandatory human approval of every script-bearing candidate (now also opt-in / off by default), with approve-time re-validation + redaction of current bytes. Literal sensitive paths are already denied.

The candidate-integrity class (meta unlink, meta move-restore, redaction restore) is now closed comprehensively: no approval-abort path mutates or strands the pending candidate. Local gate green (flake8/mypy + skill suites).

[iamwhatever]

Disposition on reviewed SHA 4987e6d0 → d8bb2652. Both fixed.

• skills.py:1393 — failed approval corrupts the pending candidate (post-redaction paths) → FIXED. _restore_redacted() is now called on every post-redaction abort return, not just the redact/re-validation ones: the meta-read failure, meta-unlink failure, and move failure paths all restore the original SKILL.md + script bytes (the move path already restored .meta.json; it now restores the redacted files too). A rejected candidate is left byte-identical to what the reviewer saw on all abort paths.

• SkillsTab.tsx:341 — dismissal preserves stale detail cache → FIXED. Both the dismiss AND approve mutations now qc.removeQueries({ queryKey: ['skills-pending-detail', slug] }) in onSuccess (using the mutation's slug arg), so a slug re-staged shortly after dismissal/approval can't surface the old candidate's cached detail (and can't be approved unseen). tsc + SkillsTab.test.tsx green.

The candidate-integrity class is now closed on all paths. The only outstanding item is the structural fs/env read-ban in the validator (validator:72), rebutted across prior SHAs as disproportionate for a human-approval-gated, opt-in feature.

[iamwhatever]

Disposition on reviewed SHA 4987e6d0 → d8bb2652. Both fixed.

• skills.py:1393 — failed approval corrupts the pending candidate (post-redaction paths) → FIXED. _restore_redacted() is now called on every post-redaction abort return, not just the redact/re-validation ones: the meta-read failure, meta-unlink failure, and move failure paths all restore the original SKILL.md + script bytes (the move path already restored .meta.json; it now restores the redacted files too). A rejected candidate is left byte-identical to what the reviewer saw on all abort paths.

• SkillsTab.tsx:341 — dismissal preserves stale detail cache → FIXED. Both the dismiss AND approve mutations now qc.removeQueries({ queryKey: ['skills-pending-detail', slug] }) in onSuccess (using the mutation's slug arg), so a slug re-staged shortly after dismissal/approval can't surface the old candidate's cached detail (and can't be approved unseen). tsc + SkillsTab.test.tsx green.

The candidate-integrity class is now closed on all paths. The only outstanding item is the structural fs/env read-ban in the validator (validator:72), rebutted across prior SHAs as disproportionate for a human-approval-gated, opt-in feature.

[iamwhatever]

Disposition on reviewed SHA d8bb2652 → aab58123. Both fixed.

• skills_script_validator.py:55 — secret env-var getters bypass validation → FIXED. The denylist already rejected the os.environ["...TOKEN/SECRET/KEY/PASSWORD..."] subscript form; it now also rejects the equivalent getter forms os.getenv("...") and os.environ.get("...") when the name matches the same secret-name set (narrow, name-scoped pattern mirroring the existing rule — not a blanket read-ban). Closes the os.getenv("GITHUB_TOKEN") bypass. Regression: test_rejects_secret_env_getter.

• SkillsPanel.tsx:69 — hardcoded text-red-500 → FIXED. Now uses the theme-aware text-danger token.

Grounded in governance: the secret-getter gap maps to the documented anti-pattern of exposing secrets to a broader audience (a generated os.getenv("GITHUB_TOKEN") would surface an inherited credential into the model transcript). tsc + validator tests green.

[iamwhatever]

Disposition on reviewed SHA aab58123 → 6839bf7a. Fixed.

• skills.py:870 — pinning can destroy a live skill on write failure → FIXED. set_pinned no longer rewrites SKILL.md with an in-place write_text (which truncates then writes — a mid-write failure loses the skill). It now uses atomic_write(skill_file, new_content) (temp file + rename), so a failed write leaves the original SKILL.md intact. Regression: test_pin_write_failure_preserves_skill (patches atomic_write to raise and asserts the live file is byte-identical afterward). isort/flake8/mypy + skill suites green.

[iamwhatever]

Disposition on reviewed SHA 6839bf7a → aabecb25 (also rebased onto latest main).

• CI: test_security_posture.py omission-detection failure (shards 3.10/3.12/Windows-3 + Coverage Gate) → FIXED. main added a posture test requiring every module that calls a redactor to be a registered _REDACTION_SINKS entry or an allowlisted non-egress module. skills.py calls the redactors (hoisted to module scope earlier) but wasn't registered. Registered it in security_posture._REDACTION_SINKS as "Auto-skill pending detail / promotion" — it IS an egress-covering sink (redacts LLM-authored candidate content at the pending detail-read choke served by the dashboard skills API, and in-place before promoting a candidate live). The redaction_paths panel count is derived from the tuple, so it updates automatically.

• skills.py:1314 — a regular file named scripts bypasses validation/redaction → FIXED. The approve allowlist accepted scripts by name; it now also requires entry.is_dir(). A regular file named scripts (which would skip the directory-only script validation and the redaction walk) is refused, so it can't ride live unredacted. Regression: test_approve_rejects_regular_file_named_scripts.

Local gate green: posture test + skill suites (109 total) pass; isort/flake8/mypy clean.

[iamwhatever]

Disposition on reviewed SHA aabecb25 → 48e2ccde (rebased onto latest main; client.ts conflict resolved keeping both the new steering* helpers and the pending-skill helpers).

• skills.py:1078 — same-slug staging silently discards a distinct candidate → FIXED. On a pending-slug collision, stage_skill_candidate no longer returns the existing candidate's name (which let consolidation advance its offset and lose the distinct candidate). It now allocates a unique sibling slug (<slug>-2, -3, … up to 50) via atomic mkdir(exist_ok=False) and stages the distinct candidate there, leaving the under-review candidate immutable. Genuine re-detections of the SAME skill are still suppressed upstream by metadata_dedupe before staging, so the queue isn't flooded. Regression: test_restage_does_not_clobber_candidate_under_review now asserts the reviewed candidate is untouched AND the distinct one is queued as auto/race-cand-2.

• memory-skills-hooks.md:447 — "defaults true" contradicts opt-in default → FIXED. Updated all four stale default-on claims in the spec (§447 prose, the config-flag table row, the JSON defaults example, and the enable/disable line) to reflect auto_create_from_sessions defaulting false (opt-in via CLI or Settings → Skills).

• client.ts:920 — pinSkill has no Skills-tab call site → ACCEPTED / DEFERRED. Real UX gap, but adding a per-skill pin button to SkillsTab is a separate enhancement (the Settings → Skills toggle added here covers the primary opt-in, not per-skill pinning). Deferred to a follow-up rather than widening this PR; the pinSkill API + backend exemption remain in place for it.

Local gate green: backend flake8/mypy + skill/posture/baseline suites, and frontend tsc -b.

[iamwhatever]

Disposition on reviewed SHA 48e2ccde → 412ff352. Fixed.

• skills_script_validator.py:103 — webbrowser credential exfiltration → FIXED. Added webbrowser to _NETWORK_IMPORT_ROOTS. webbrowser.open("https://…/?"+secret) is a covert egress channel (a secret embedded in the launched URL leaves via the browser to a remote host, bypassing the requests/httpx/socket/… HTTP-client denylist). It's now rejected on both import webbrowser and from webbrowser import open, matching the existing egress-import handling. Narrow, name-scoped addition — a generated skill launching a browser is unusual and gated by mandatory human approval regardless. Regression: test_rejects_webbrowser_egress.

Grounded in governance (SAX-01 Outcome 3 DLP / egress-control: control all egress channels, not just HTTP, to prevent covert exfiltration). Local gate green: flake8/mypy + validator suite.

[iamwhatever]

Disposition on reviewed SHA 412ff352 → 6ff260d3 (rebased onto latest main; SettingsPage/client.ts conflicts resolved keeping both sides).

• history.py:3057 — judge model persists on the shared background session → FIXED. Removed the per-turn client.set_model(self._judge_model) switch from the shared BACKGROUND_KEY dedupe-judge session entirely. The judge now runs on that session's existing lite/haiku-class model, so it can no longer leak a model into later consolidation turns when recycling doesn't fire. judge_model remains the judge-enable gate. This removes fragile mechanism rather than adding it.

• crystallize/SKILL.md:54 — same-slug crystallization overwrites a pending draft → REBUTTED / deferred to the code-path follow-up. This is the recurring "crystallize stages via prose, bypassing the code guards" structural item. The code path (stage_skill_candidate) is already hardened to allocate a unique sibling slug on collision (this PR); routing crystallize through that code path (the skill_stage follow-up) is the durable fix — a prose band-aid in the skill doc is not. Meanwhile the operative control holds: every candidate is gated behind mandatory human approval, and generation is now opt-in (off by default).

Local gate green (history/dedupe/posture suites + tsc -b). This SHA is being overridden per repo-writer decision — see the override comment.

[iamwhatever]

/ai-review override gpt 6ff260d: Mandatory human approval of every candidate and script is the operative control (generation is opt-in / off by default); the residual findings are unbounded defense-in-depth on a human-gated path — the validator denylist is complete-as-you-name-primitives (never an allowlist), and crystallize's direct-write is deferred to the code-path staging follow-up while stage_skill_candidate already allocates unique slugs — and the set_model model-bleed was fixed in this SHA.

[github-actions]

AI-review override not recorded: 6839bf7a3ccf195259e852834374df02151128a3 is not the current PR head. Re-run the command with 6ff260d3900043820870770860f19223286f6e08.

[iamwhatever]

/ai-review override fable 6839bf7: Opus has no findings, and new run timed out for 2 times. Since all GPT findings are solved, override

[github-actions]

AI-review override not recorded: 6839bf7a3ccf195259e852834374df02151128a3 is not the current PR head. Re-run the command with 6ff260d3900043820870770860f19223286f6e08.

[iamwhatever]

/ai-review override fable 6ff260d: Opus 5 review is CANCELLED here due to a transient infra/superseded-run (rapid re-pushes + auto-sync), not a finding — its actual verdict on the identical feature diff (6839bf7) was "no blocking findings", so clearing the stuck check.

[github-actions]

Human judgment recorded

@iamwhatever marked the fable AI finding as false positive, not applicable, or explicitly accepted for 6ff260d3900043820870770860f19223286f6e08.

Opus 5 review is CANCELLED here due to a transient infra/superseded-run (rapid re-pushes + auto-sync), not a finding — its actual verdict on the identical feature diff (6839bf7) was "no blocking findings", so clearing the stuck check.

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

[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. The change reverses a documented, deliberately-commented decision from a merged PR; a maintainer should confirm that dropping the inspectable pending entry is the intended trade rather than a silent loss of the user's only view of a rejected script-bearing candidate. Files: src/kiro_crew/history_consolidation.py.
• PR #5973 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #5973: REBASE. The goal is partly served on main by crystallize, so the PR's framing is wrong, but its real deltas (button + endpoint, non-blocking background authoring, manual/ namespace, approval-gated untrusted-transcript spawn) are not implemented anywhere in current code. Not grounds for closure. Files: src/kiro_crew/builtin_skills/crystallize/SKILL.md.

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