fix(skills): respect disabled approval setting

[rubencu]

Problem / Motivation

skills.approval_required=false could still produce New skill awaiting review notifications through two paths:

1. The dashboard persisted the setting but the running HistoryConsolidator kept the value captured at gateway startup.
2. A generated candidate that supplied scripts was always staged even when every script failed validation and was discarded, leaving a saved candidate with has_scripts: false that still bypassed the user's opt-out.

Why it matters

The Settings UI reports that approval is disabled while users still receive approval requests for candidates containing no reviewable scripts. The notification even offers to stop requiring approval although that setting is already off.

What changed (motivation → approach → change)

• Hot-reload skills.approval_required into the active consolidator after a successful dashboard config write, matching other startup-cached settings.
• When approval is disabled and a candidate supplied scripts but all fail static validation, reject the candidate as a whole. It is neither auto-published nor added to the pending review queue, so no contradictory approval notification is emitted.
• Preserve both safety boundaries: a candidate retaining any valid script always stages, and an all-invalid candidate still stages when approval is enabled so it can be manually inspected.

Tests

• Added a parameterized config PATCH regression proving both true → false and false → true update persisted config and the running consolidator.
• Updated the all-invalid-script regression to require neither a live skill nor a pending review when approval is disabled.
• Adjacent coverage proves valid-script candidates still stage with approval disabled and all-invalid candidates still stage when approval is enabled.
• Affected test files: 400 passed.
• Formatting, isort, flake8, mypy, and deterministic backend guard scripts passed.
• Mutation proof: the exact invalid-script regression fails when the production rejection hunk is removed.
• The prior PR head's complete cross-platform CI matrix passed before this amendment.

Manual verification

N/A — unit coverage exercises persisted config, in-memory runtime state, script validation, live-skill creation, and pending-queue behavior directly.

Related Issues

No linked issue: reported directly from the dashboard behavior.

Pattern harvest

Rule candidate: review-prompt
Pattern: Dashboard config patches for startup-cached settings must update the corresponding live service after persistence.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (not applicable — no contract or user-facing text changed)
• No secrets, credentials, or internal references in the diff

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] be48b6d

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

The change is a minimal hot-reload block that mirrors the established completion_keep convention directly above it. cfg = KiroCrewConfig.load() re-reads the validated config, request.app["state"] is always present (used unguarded elsewhere in the function), the state.consolidator is not None guard covers absence, a bool write is atomic under the GIL, and the comment's claim about script-bearing candidates matches the or valid_scripts or scripts_supplied guard at history_consolidation.py:1756. No grounded defect on the changed lines and no rule violation.

No findings.

[OPUS-REVIEWED] be48b6d

[github-actions]

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

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

All evidence is in. The fix targets the single consolidator instance the gateway process holds (the orchestrator passes it into the dashboard state), follows the handler's established hot-reload pattern, and the description matches the diff. One counted sibling remains unfixed.

First-Principles-Verdict: CONCERNS

The fix is real and rightly shaped, but it patches one of exactly two consolidator-snapshotted patchable keys — skills.auto_create_from_sessions stays stale.

What this change ships

Intent: make the dashboard's Skill Approval Required toggle take effect immediately instead of after a gateway restart — a FIX.

1. Toggling skill approval now changes running behavior at once — justified; follows the handler's existing hot-reload pattern (completion_keep, core.py:2205).
2. A gateway log line records the hot-reload — rides along; matches every sibling hot-reload block.

Watch

• Point patch with 1 counted unfixed sibling. Grep of the PATCH key table for consolidator-fed keys finds exactly two: skills.approval_required and skills.auto_create_from_sessions (core.py:1742). The sibling is snapshotted identically (_auto_skills_enabled, history_consolidation.py:365, read at :1008) and exhibits the same defect — Settings says auto-creation is on, the running consolidator never generates until restart. The same one-line sync in the same handler block closes it; as shipped, the root condition ("patchable key snapshotted at consolidator construction") survives.
• The description's claim "the config PATCH handler already hot-reloads other startup-cached settings" is verified true (5 existing per-key blocks), so the shape is a recorded repo decision — but note the precedents use a public method (update_completion_keep) where this writes the private _approval_required directly; that shape call belongs to design review, not this lane.

[FIRST-PRINCIPLES-REVIEWED] be48b6d

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound fix following the handler's established hot-reload pattern, but it drops the part of that pattern that keeps it robust: the public setter.

Watch

• The hunk writes state.consolidator._approval_required directly, while the sibling hunk it emulates goes through a public method (state.subagents.update_completion_keep(...)). Combined with the test stubbing the consolidator as SimpleNamespace(_approval_required=...), a later rename of that private field inside HistoryConsolidator leaves both the handler and the test green while the stale-approval bug silently returns — the write just creates an orphan attribute.
• The other toggle in the same Settings panel, skills.auto_create_from_sessions (SkillsPanel.tsx:70), is snapshotted identically (auto_skills_enabled at construction) and gets no hot-reload — turning auto-create off keeps generating/staging skills until restart, the exact harm this PR describes. Your own harvest states the general rule; this ships one instance of it. Fine as a follow-up, but a human should know the class is still open.

Suggestions

• Add a small public setter on HistoryConsolidator (mirroring update_completion_keep) and assert through it — or a real consolidator — in the test, so a rename can't silently reopen the bug.

[DESIGN-REVIEWED] be48b6d

[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

• This PR is OVERLAPPING with PR #392. 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.
• This PR is OVERLAPPING with PR #2713. 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.
• This PR is OVERLAPPING with PR #4533. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7617: MERGE_DISCUSSION. No code conflict, but the two open PRs encode opposite answers to the same question — whether a candidate that supplied only invalid scripts must remain inspectable. One rule should be settled before either lands. Files: src/kiro_crew/history_consolidation.py, src/kiro_crew/dashboard/handlers/core.py.
• This PR is OVERLAPPING with PR #6307. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7617: MERGE_DISCUSSION. Independent keys in a shared handler; at most a mechanical rebase, no behavioural interaction. Files: src/kiro_crew/dashboard/handlers/core.py.

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