--- name: review-criticals-1e39e36dbc70 description: Audit every Critical section for necessity, scope, and duration — find stale or overly broad holds that block the main thread user-invocable: true disable-model-invocation: true --- 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 ` 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. --- Source attribution: cwilliams5 Original project: https://github.com/cwilliams5/Alt-Tabby/blob/6e941fe6d8344facc4f24c373f41999ff6da4c04/.claude/skills/review-criticals/SKILL.md License: MIT License information: https://opensource.org/license/mit Original upstream instructions; downloaded from Dave AI. ## Upstream license notice MIT License Copyright (c) 2025 Cody Williams Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions: The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. # Dave AI resource metadata Creator / publisher: cwilliams5 Original project: https://github.com/cwilliams5/Alt-Tabby/blob/6e941fe6d8344facc4f24c373f41999ff6da4c04/.claude/skills/review-criticals/SKILL.md Attribution basis: Source repository record Fields of work: Development & Engineering Roles: Backend Developer, DevOps / SRE, QA / Test Engineer Specialties: Backend Tools: Ahk Country applicability: Country not specified Classification basis: Source catalog labels Instruction SHA-256: 5ce26b8efc9ded22b54a111af3eb4eeffa82bb4843271a98c3c679f13a70921b