# Review Criticals

> Audit every Critical section for necessity, scope, and duration — find stale or overly broad holds that block the main thread

- Skill: `majiayu000/review-criticals` (Agent Skill, multi-file: 2 files)
- Install (CLI): `npx skillmds add majiayu000/review-criticals`
- Raw SKILL.md: https://api.skillmd.com/api/skills/majiayu000/review-criticals/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security
- Author: majiayu000 (https://skillmd.com/u/majiayu000)
- Updated: 2026-09-09
- Page: https://skillmd.com/skills/majiayu000/review-criticals

---

Enter planning mode. Two-phase audit: first inventory every Critical hold in the codebase, then evaluate each for necessity and scope. Use maximum parallelism — spawn explore agents for independent file groups.

## Why This Matters

`Critical "On"` prevents timer, hotkey, and callback interruption — it's the only synchronization primitive in AHK v2. But every Critical hold **blocks the main thread**. Keyboard hooks, timer callbacks, and WinEvent callbacks all queue up behind it. Critical sections accumulate over time as defenses against bugs, but the conditions that required them can change: a refactor moves shared state into a single producer, a Map iteration gets replaced with a snapshot copy, yet the Critical stays.

This is the mirror of `review-race-conditions`:
- `review-race-conditions`: "where should Critical exist but doesn't?" (additive)
- `review-criticals`: "where does Critical exist but shouldn't, or is wider than needed?" (subtractive)

## Phase 1 — Inventory

Find every `Critical "On"` / `Critical "Off"` pair in `src/core/` and `src/gui/` (plus `src/shared/` files they include). For each hold, document:

| Function | File:Lines | Protected Code Summary | Hold Duration | Shared State Touched |
|----------|-----------|----------------------|---------------|---------------------|
| `_WEH_WinEventProc` | `winevent_hook.ahk:196+` | Check eligibility, upsert window, update MRU | Medium (~50-200μs) | `gWS_*` store globals |

**Hold Duration estimates:**
- **Trivial** (~1-5μs): Single assignment, counter increment, flag set
- **Short** (~5-50μs): Few property accesses, simple if/else, Map Has+Set
- **Medium** (~50-500μs): Loop over small collection, multiple DllCalls, string building
- **Long** (~500μs+): Loop over unbounded collection, file I/O, large data structure rebuild

Also note any `Critical "On"` without a matching `Critical "Off"` on every exit path (early returns, continues). These are bugs regardless of whether the Critical is necessary — but that's `review-race-conditions` territory. Just flag them as cross-references.

## Phase 2 — Evaluate Each Hold

For each Critical section, answer one question: **What specific race would occur if this Critical were removed?**

This is not "is this a good idea" — it's "name the exact interleaving." What interrupt source fires, at what point in the protected code, modifying what shared state, causing what corruption?

### Classification

**Still necessary** — A concrete race exists:
- Name the interrupt source (which timer, hotkey, or callback)
- Name the interleaving point (between which two operations)
- Name the corruption (stale read, lost write, partial state, collection modified during iteration)
- Verdict: keep as-is

**Narrowable** — The Critical protects more than it needs to:
- The hold spans N lines but only M lines (M < N) contain the actual shared state access
- Work before or after the critical shared state access could be moved outside
- Computations, local variable setup, string formatting, DllCalls that read (not write) local state — these don't need interrupt protection
- Verdict: shrink the scope, move non-critical work outside

**Stale** — The condition that required this Critical no longer exists:
- The protected variable is no longer shared (only one callback type accesses it now)
- The dangerous interleaving was eliminated by a refactor (e.g., Map iteration replaced with snapshot copy that happens outside Critical)
- The function is no longer called from an interruptible context
- Verdict: remove

**Defensive but unverifiable** — Can't name a specific race, but:
- The pattern *looks* like it could race in theory
- The code has a comment explaining the Critical but the explanation references old architecture
- Similar code elsewhere uses Critical for the same pattern
- Verdict: flag for human review with honest "I cannot name the specific race" assessment

## Known Safe — Do NOT Flag

These Critical sections have been deliberately designed, tested, and documented. Do not propose removing, narrowing, or questioning them:

- **`Critical "On"` through the entire `GUI_OnInterceptorEvent` handler including GDI+ rendering** (~16ms). This was previously narrowed and reverted — releasing before render causes partial glass background, window mapping corruption, and stale projection data. See `keyboard-hooks.md` "CRITICAL: Critical Sections During Rendering (Context-Dependent)." No internal abort points — unsafe to interrupt. This is the most important Critical section in the codebase.
- **`Critical "On"` in `_INT_Alt_Down`, `_INT_Alt_Up`, `_INT_Tab_Down`, `_INT_Tab_Up`, `_INT_Tab_Decide`, `_INT_Ctrl_Down`, `_INT_Escape_Down`** — all hotkey callbacks require Critical to prevent callback-interrupting-callback corruption.
- **`Critical "On"` in `_Anim_FrameLoop`** — covers `_Anim_UpdateTweens()` → `GUI_Repaint()` → DComp sync. Phase 1 (#177) deliberately moved the long blocking wait (compositor clock / waitable swap chain) OUTSIDE Critical so the AHK message pump runs during the wait. The Critical section is only held during the render + present + commit sequence. This is the intended design.
- **`Critical "Off"` before `_GUI_ShowOverlayWithFrozen()` in `_GUI_GraceTimerFired`** — must release before the 1-2s first paint or Windows' `LowLevelHooksTimeout` (~300ms) silently removes hooks (#303). This is safe because the show path has 3 RACE FIX abort points that detect `gGUI_State != "ACTIVE"`. This is the deliberate opposite of `GUI_OnInterceptorEvent` — context-dependent rule, see `keyboard-hooks.md`.
- **Async activation event buffer** (`gGUI_EventBuffer`) — Critical around buffer push/pop is necessary by design.
- **Flight recorder `FR_Record()`** — if it uses Critical, it's protecting the ring buffer write pointer. Pre-allocated, intentional.

If you encounter these, classify as "Still necessary — known safe, documented" and move on. Spend your tokens on the holds that *aren't* on this list.

## Explore Strategy

Split by file group (run in parallel):

- **Producer callbacks** — `src/core/winevent_hook.ahk`, `komorebi_sub.ahk`, `komorebi_state.ahk`, `komorebi_lite.ahk`, `mru_lite.ahk`, `winenum_lite.ahk`
- **GUI hot path** — `src/gui/gui_state.ahk`, `gui_interceptor.ahk`, `gui_input.ahk`, `gui_data.ahk`, `gui_paint.ahk`
- **GUI support** — `src/gui/gui_overlay.ahk`, `gui_pump.ahk`, `gui_workspace.ahk`, `gui_win.ahk`, `gui_monitor.ahk`
- **Shared infrastructure** — `src/shared/window_list.ahk`, `blacklist.ahk`, `ipc_pipe.ahk`, `stats.ahk`

Use `query_timers.ps1` to cross-reference — timer callbacks are the primary interrupt source for non-hotkey Critical sections. Use `query_mutations.ps1 <globalName>` to see which functions mutate a given global — reveals whether all writers are inside Critical sections without reading full files.

## Validation

After explore agents report back, **validate every finding yourself**. Removing or narrowing a Critical section is high-risk — a wrong call creates a race condition that may only manifest under specific timing.

For each candidate:

1. **Cite evidence**: "I verified by reading `file.ahk` lines X–Y" with the protected code quoted. Show what's inside the Critical boundaries.
2. **Name the race (or admit you can't)**: For "still necessary" — state the specific interleaving. For "stale" or "narrowable" — explain why the race no longer exists or why the moved code is safe outside Critical. For "defensive but unverifiable" — be honest that you can't name the race.
3. **Check interrupt sources**: Use `query_timers.ps1` and grep for `SetTimer` / `Hotkey(` to identify what could actually interrupt this function. If nothing can interrupt it (function is only called from within an already-Critical context), the Critical may be redundant.
4. **Check callers**: A Critical section might look unnecessary in isolation but be required because callers invoke it from non-Critical contexts. Use `query_function_visibility.ps1` to check all call sites.
5. **Counter-argument**: "What timing scenario would make removing/narrowing this Critical dangerous?" — Even if you can't name one, consider: could a future change re-introduce the race? Is the Critical cheap enough that the safety margin is worth it?

## Plan Format

**Section 1 — Inventory:**

| # | Function | File:Lines | Hold Duration | Shared State | Classification |
|---|----------|-----------|---------------|-------------|----------------|
| 1 | `_WEH_WinEventProc` | `winevent_hook.ahk:196+` | Medium | `gWS_*` | Still necessary |
| 2 | `KSub_CacheFocusedHwnds` | `komorebi_state.ahk` | Long | `gKS_Cache`, `gWS_*` | Narrowable |
| 3 | `_OldHelper` | `some_file.ahk:30-35` | Trivial | `gFoo` (no longer shared) | Stale |

Total: N Critical sections. X still necessary, Y narrowable, Z stale, W unverifiable.

**Section 2 — Narrowable holds (recommended changes):**

| Function | File:Lines | Current Scope | Proposed Scope | Lines Moved Out | Est. Time Saved | Risk |
|----------|-----------|--------------|---------------|----------------|----------------|------|
| `KSub_CacheFocusedHwnds` | `komorebi_state.ahk` | 60 lines | 15 lines | JSON parse (before), log write (after) | ~100μs | Low — moved work is purely local |

**Section 3 — Stale holds (recommended removal):**

| Function | File:Lines | Why Stale | Evidence |
|----------|-----------|----------|---------|
| `_OldHelper` | `some_file.ahk:30-35` | `gFoo` is only written by this function now (confirmed via `query_global_ownership.ps1`) | No other writers, no timer/hotkey accesses this path |

**Section 4 — Defensive/unverifiable (human review needed):**

| Function | File:Lines | What It Protects | Why Unverifiable | Recommendation |
|----------|-----------|-----------------|-----------------|----------------|
| `_SomeFunc` | `file.ahk:90-95` | `gBar` increment | Only one known interrupt source, but pattern suggests historical race | Keep — trivial duration, safety margin worth it |

Order by hold duration within each section (longest first — the most blocking holds are the highest value targets for narrowing/removal).

Ignore any existing plans — create a fresh one.