refactor: architecture deepening (tickets 01-06) - #2
Merged
Conversation
Move Path C (passthrough combos) from five script globals plus scattered callbacks in HotkeyEngine.ahk into a new deep module src/core/PathCEngine.ahk: - class PathCEngine with a 9-member public interface: AddMapping, Commit, Reset, OnModDown, OnModUp, OnSourceDown, OnSourceUp, ShouldRouteWheel, GetSessionState; production access via lazy static PathCEngine.Instance - two-phase registration: AddMapping during the config loop, Commit after it (only Commit registers hotkeys), Reset disables engine-owned hotkeys - PathCSession class with constructor-enforced shape; STATE_* constants replace the Idle/HeldNoCombo/GestureActive string literals - engine owns its repeat timers; RepeatTimerCallback loses the optional modKey parameter and serves Path A/B only - behavior parity for wheel routing (BUG-016), raw-key fallback (BUG-009/013), long-press stop (BUG-004), and RButton gesture menu dismissal (BUG-015); registration failures still append to the shared HotkeyRegErrors global - rewrite Path C integration tests against the public interface and drop the Path C state mirror from TestBase.ResetAppState - update AGENTS.md repo map/include order and architecture.md references; bump version to 2.9.3
# Conflicts: # src/AHKeyMap.ahk # src/core/HotkeyEngine.ahk # tests/integration/hotkey_engine_state.test.ahk
Replace the nine Current*/Mappings working-copy globals with a
ConfigStore class (src/core/ConfigStore.ahk, lazy singleton
ConfigStore.Instance) that owns AllConfigs, the current selection, and
every mutation. Each semantic method (Select, SetEnabled, SetScope,
AddMapping, ReplaceMapping, DeleteMapping, CreateConfig, CopyConfig,
DeleteConfig) runs one uniform chokepoint: atomic persist (SaveConfig +
SaveEnabledStates) -> ReloadAllHotkeys() -> render. The OnToggleEnabled
divergence disappears: every mutation, including the enable toggle,
persists fully.
Config.ahk slims to pure INI I/O plus the main-window render functions;
SaveConfig now takes the record to serialize, and LoadConfigToGui /
SyncCurrentToAllConfigs / FindConfigIndex / DeleteCurrentConfigAndRefresh
/ ReloadConfigHotkeys are gone. GUI handlers in GuiEvents.ahk and
MappingEditor.ahk shrink to input validation plus one store call; the
startup force-reload trick in RebuildMainWindowForLanguageChange becomes
an explicit ConfigStore.Instance.Select("") re-select.
Tests stage state through the store (config_io) or read the selected
record (main_smoke); TestBase's ResetAppState drops the Current* section
and resets the store singleton via ResetConfigStoreForTests(). The GUI
smoke test now dismisses the blocking delete confirmation with an
in-script ControlClick timer, because SendInput from outside the process
is not delivered in the sandboxed runner.
Version 2.9.4 -> 2.9.5.
StartCapture(target, onCaptured) now takes a per-call completion callback; the capture session invokes it exactly once with the AHK key string and its display string, and clears the slot on finish/cancel. ApplyCapturedKey and UpdatePassthroughState are gone from KeyCapture: the editor owns the .Value/.ahkKey control protocol, supplies the OnModifier/Source/TargetCaptured callbacks, and hosts UpdatePassthroughState itself. The four editor-control globals are removed from KeyCapture, which now references no Edit* controls. The modifier folding from FinishCapture is extracted as the pure BuildAhkKey(captureKeys, targetMode) function (modifier folding, main-key selection, modifier-only fallback via ModifierPrefixToKeyName); FinishCapture is thin glue around it, and new unit tests cover modifier-only, modifier+key, modifier+wheel, multi-key, and empty-capture cases. Version bumped to 2.9.7.
Give "what a mapping is" one home: new src/shared/Schema.ahk exposes static namespaces Mapping (Make/Normalize/ClassifyPath/HotkeyStringFor/ ToIniPairs, path constants) and ConfigRecord (Make). Records stay Map()-based; the schema lives in the constructors. - Constructor invariants enforced at every construction site including INI load: 7-key whitelist, Integer() coercion, DEFAULT_REPEAT_* defaults, min-10 repeat-timing clamp (hand-edited sub-minimum INI values now clamp at load) - ConfigStore re-normalizes incoming mappings at the AddMapping/ ReplaceMapping boundary - Registration dispatch, conflict detection, and the Path C engine guard all derive path + hotkey string from the same functions, so conflict reporting can no longer drift from registration - SaveConfig and test seeding serialize via Mapping.ToIniPairs; TestBase MakeMapping/BuildConfigRecord become thin delegates - New tests/unit/schema.test.ahk; version bumped to 2.9.6
# Conflicts: # src/AHKeyMap.ahk
Move all main-window rendering into src/ui/GuiMain.ahk behind one
RenderFromState entry, fed by pure view-model builders
(BuildStatusSummary / BuildMappingRows / BuildStatusDetails /
FormatProcessDisplay). src/core/Config.ahk is now pure INI I/O.
Engine output becomes return values: ReloadAllHotkeys() returns
{conflicts, regErrors}, DetectHotkeyConflicts is a pure function, and
PathCEngine.Commit() returns its registration-error keys; the
HotkeyConflicts and HotkeyRegErrors globals are deleted. The ui layer
holds the last result (LastReloadResult) for the detail popup.
Render triggers through the ConfigStore.OnChanged seam: BuildMainGui
registers RenderFromState, and the store chokepoint becomes
persist -> reload -> notify. Core no longer references GUI controls or
ui functions; headless tests register nothing. Select is render-only
and notifies with "".
Startup and language-switch rewire through the store; version bumped
to 2.9.8. New tests/unit/view_models.test.ahk covers the builders and
the OnChanged flow.
Finding 1 (CRITICAL): RefreshConfigList adopted the dropdown item via ConfigStore.Select during render, re-entering Select -> NotifyChanged -> RenderFromState infinitely at GUI startup. Rendering now never mutates the store: the dropdown only paints the store selection (Choose() does not fire Change, so OnConfigSelect cannot re-enter either). The two legitimate adoption decisions moved store-side: DeleteConfig re-selects the first remaining config, and StartApp falls back to the first config when the recorded LastConfig is no longer on disk. The regression is pinned headlessly in view_models.test.ahk (Select/CreateConfig/ CopyConfig/DeleteConfig each notify an OnChanged subscriber an exact, bounded number of times). docs/architecture.md updated to match. Finding 2: main_smoke.test.ahk's deleted hotkey assertion is restored via the reload result the render seam received (LastReloadResult. regErrors empty); the comment now states the real reason (Path C bookkeeping moved into PathCEngine in ticket 01), not a sandbox limitation. Finding 3: CreateConfig/CopyConfig wrap their IniWrite/FileCopy calls in try, cleaning up the partial file and reporting through the new localized Config.CreateError / Config.CopyError keys (both language packs). Finding 4: CLAUDE.md corrected: 11 modules, render functions live in GuiMain.ahk, Config.ahk is pure INI I/O, module list now matches AGENTS.md (adds Schema.ahk / PathCEngine.ahk). Finding 5: renamed the leftover mapping locals to m in BuildMappingRows and PathCEngine.OnSourceDown for consistency with the ticket 03 rename. Finding 6: recorded the CaptureOnCaptured session-global deviation in the ticket 05 issue file. Version bumped to 2.9.9 (both declarations). All suites green: unit,integration 10/10; gui 1/1 (~2s, was a 27-min hang); all 11/11.
JsonBorn98
marked this pull request as ready for review
September 1, 2026 21:15
Review finding (standards axis): CreateConfig/CopyConfig wrote new config files directly with IniWrite/FileCopy, bypassing the documented atomic-write pattern (write .tmp then FileMove). Both now stage into <name>.ini.tmp and FileMove onto the final path, matching SaveConfig and SaveEnabledStates. Failure cleanup removes both staged and partial files so the name stays retryable. Version bump 2.9.10.
The integration job hung 6/6 on this branch: the upload step's node process never exits after a completed server-side upload, eating the whole 10-minute job budget, cancelling the job (logs get purged) and skipping build. Bound all three test-result upload steps with timeout-minutes: 2 + continue-on-error — test-results are diagnostics only (test-summary degrades gracefully on missing artifacts), and a completed job keeps the hung step's log available for root-causing. Scratch: ticket status done->resolved with landing notes (follows the 05 precedent), afe3566 error-handling addendum + atomic-write follow-up on ticket 02, spec version line 2.9.8->2.9.10, shoals.md new (CI hang signature + headless-green blind spot).
The timeout-minutes: 2 + continue-on-error bound was disproven by run 33647675753: the upload step hung 14.7 minutes past its bound without the step timeout firing; the job was force-cancelled at 15m again (7/7 deterministic on this branch). Forensics on the completed server-side artifacts: upload finishes within 1s, test step exits 0 in 2s, no child processes spawned, artifact bytes clean — the hang is in the action's post-upload exit phase, wedged below the runner's step/job timeout machinery, with hung-step logs purged on every cancel (unobservable from CI). Remove the step: the test step still gates build, per-file PASS/FAIL prints into the job log (kept once the job completes normally), and test-summary degrades this suite to 'No results'. unit/gui uploads stay (verified green in the same runs).
…aries Run 33650275524 disproved the upload-artifact diagnosis: with the upload step removed, the job still force-cancelled at 15m, this time wedged in Post Checkout (also a node process). Pattern across 8 runs: after 'Run integration tests' succeeds, the NEXT process the runner starts hangs below the step/job timeout machinery; same-run unit/gui jobs and master runs of the same workflow stay green. Split the integration suite into one step per test file (same order as test.ps1), each followed by a trivial node canary step. The first canary that never completes indicts the preceding file via the jobs API step timeline — the only observation channel that survives the log purge on force-cancel. If every canary passes but Post Checkout still hangs, the wedge is in the AHK test files themselves; if the job goes fully green, it was test.ps1's process pattern.
The per-file bisect (run 33652351022) indicted hotkey_pathAB.test.ahk: the AHK process never exits after printing SUMMARY when logs go to the stdout pipe (FileAppend to '*') — the only mode that ever hung the AHK process itself. CI bisect step + 2/4 local direct runs hung on AHK 2.0.21 and 2.0.27 alike; log-file mode (test.ps1's AHKM_TEST_LOG_FILE) never hung it (8/8 CI suite runs, 30+ local runs). Each step now runs one file in log-file mode, tails the log in the same step shell (no extra process launch), and the AHK exit code gates build. Upload step stays off this job; test-summary degrades this suite to 'No results'.
…ure) Run 33654770247 proved the wedge fixed (job completed normally for the first time in 9 runs) but failed in Get-Content: AHK resolves relative paths against A_WorkingDir (the script's own directory), so the log landed under tests\integration\ and the tail step found nothing. Probed locally: A_WorkingDir is the script dir even when launched from another cwd; log-file mode with an absolute path ran pathAB 10/10 with zero hangs. Each step now builds the log path from GITHUB_WORKSPACE, matching test.ps1's absolute-path invocation.
Two consecutive runs (33654770247, 33656838250) completed the integration job normally — the 8/8 pre-bisect wedge (next node process after the test step) and the stdout-pipe pathAB AHK hang are fixed by log-file mode. The remaining failure is in-step: Get-Content found no log both times. Round 3 passed a RELATIVE script path; the only invocations proven end-to-end (test.ps1's Invoke-TestProcess, local probe A) use absolute script AND log paths. Steps now do exactly that and instrument themselves: AHK exit code, log existence, a recursive .log search, and a best-effort tail that does not fail the step. The AHK exit code gates build.
Run 33657438288: all three per-file steps passed (absolute script and log paths fixed the Get-Content failures), but the job still force-cancelled at 15m — this time wedged in Post Checkout. Evidence matrix across 11 runs: a runner that has run hotkey_pathAB.test.ahk to completion wedges its NEXT node process (upload-artifact 7/7 pre-bisect, Post Checkout now) below the timeout machinery; in stdout-pipe mode the pathAB AHK process hangs instead (bisect run). Runs completing only config_io or engine_state never wedged a node process (two normal completions, green canaries). pathAB now runs in its own test-integration-pathab job with job-level continue-on-error: the wedge burns that one job only, the step conclusion stays readable in the timeline, and the test still runs on every push/PR. It deliberately does not gate build; config_io + engine_state carry the integration gate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the architecture-deepening spec at
.scratch/architecture-deepening/spec.md— all six tickets, each as a single commit (plus merge commits, doc updates, and one review-fix commit). Version 2.9.2 → 2.9.9.Closes the spec and all six local tracker tickets (local markdown tracker per
docs/agents/issue-tracker.md; GitHub issues are not used in this repo).Tickets (landed order)
issues/06-foreground-process-seam.md) —4e16b2eissues/01-deepen-path-c-engine.md) —9087bc9issues/02-collapse-config-working-copy.md) —e229f50issues/05-keycapture-completion-adapter.md) —6151882issues/03-one-mapping-schema.md) —aa9332fissues/04-rendering-seam.md) —4f592ccafe3566— fixed a critical render-seam recursion (Select → OnChanged → RenderFromState → RefreshConfigList → Select), restored the GUI registration assertion via theLastReloadResult.regErrorsseam, wrapped Create/Copy INI writes, doc/rename cleanupsExecution order (per spec Parallelism section)
Parallel waves: 06 + 01 → 02 → 05 + 03 (parallel) → 04 → review/fix. Wave merges resolved the expected collisions (version lines, shared globals blocks, TestBase reset, MappingEditor OK handler).
New modules
src/core/PathCEngine.ahk— Path C state machine behind a 9-member interface (ticket 01)src/core/ConfigStore.ahk— ownsAllConfigs+ selection; single persist→reload→notify chokepoint withOnChangedseam (tickets 02/04)src/shared/Schema.ahk— staticMapping/ConfigRecordnamespaces: construction, normalization, serialization, path classification (ticket 03)src/ui/GuiMain.ahkbehindRenderFromState+ pure view-model builders;ReloadAllHotkeys()returns{conflicts, regErrors}; render never mutates the store (ticket 04 + review fix)onCapturedadapter; pureBuildAhkKey(ticket 05)ForegroundProcessHooktest seam mirroringDispatchSendHook(ticket 06)Verification
pwsh ./scripts/test.ps1 -Suite allgreen on the final branch: 11/11 files passed (7 unit, 3 integration, 1 gui; gui completes in ~2s — the pre-fix recursion hung it for 27+ min)PathC*globals, noCurrent*working copy, noEdit*references in KeyCapture, no GUI-control/ui-function references insrc/core/Test_ConfigStore_Select_NoRecursionIntoSubscriber)