Skip to content

Fix infinite iteration in keybinding scanner API stubs - #12712

Open
wulujia wants to merge 3 commits into
omacom:quattrofrom
wulujia:fix/keybindings-noop-iteration
Open

wulujia wants to merge 3 commits into
omacom:quattrofrom
wulujia:fix/keybindings-noop-iteration

Conversation

@wulujia

@wulujia wulujia commented Sep 21, 2026 •

Copy link
Copy Markdown

The keybinding menu can spin indefinitely when a user's Lua config iterates an API query such as ipairs(hl.get_loaded_plugins()). The scanner's no-op object's __index returns itself for every numeric index, so the iterator never sees nil. Repeated menu launches can leave multiple Lua processes consuming one CPU core each.

Return nil for numeric lookups while preserving chained attribute access and calls. This lets query results act as empty lists during the offline scan.

The regression test loads a plugin through the stub, iterates the plugin query, then checks that subsequent Lua bindings are still resolved. A five-second timeout bounds the test if the bug returns.

Validation:

  • The new regression times out on the original code and passes with the fix.
  • bash test/shell.d/keybindings-menu-test.sh passes.
  • The corrected embedded scanner finishes against the affected machine's actual config.
  • Bash syntax checks and git diff --check pass.
  • ./test/all completed: CLI passed; 236 of 245 shell test files passed. Eight failing files also fail on the untouched base commit (0d5232ed) in this environment: config, factory-reset-accounts, launch-about, network-qr, omarchy-kernel-migration, snapper, unowned-system-paths, and update-pacman. The packaging checks require an additional omarchy-pkgs checkout. The ninth failure, locate, tries to decode a generated bin/__pycache__ file as UTF-8; it passes after moving that test-generated cache out of the checkout, and passes on the clean base checkout.

Fixes #7025

Fixes #12124

Fixes #14243

Fixes #14302

@llstrk

llstrk commented Sep 23, 2026

Copy link
Copy Markdown

Verified: At 0baf115b, the changed offline Lua scanner terminates the reported plugin-list iteration and discovers the binding declared after it. No important actionable defect was identified in this scoped change.

For a synthetic config containing ipairs(hl.get_loaded_plugins()) followed by hl.bind(...):

Scanner Numeric lookup Observed result
Base Returns the no-op object repeatedly Reached the timeout without emitting the later binding
PR Returns nil at index 1 Exited successfully and emitted the later binding

String-property chaining and calls also retained the expected binding output. Source inspection confirms the new regression checks later binding resolution, not just whether the command exits.

Compatibility limit: Empty offline lists do not emulate populated API results. In a synthetic config, unguarded hl.get_loaded_plugins()[1].name now errors and stops discovery of subsequent bindings. No shipped config using that direct-index pattern was identified; supporting it would require a different query-emulation contract, not merely changing the iteration assertion.


Review information

Test scope: Pinned-source review and isolated execution of the unchanged embedded Lua scanners with synthetic configs, including positive controls and a bounded baseline timeout. This does not establish full-suite success, behavior with the reporter's actual config, real plugin state, or interactive menu dispatch.

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Automated AI review: Astra Medium performed initial inspection and synthesis; Opus 5.5 High and GPT 6 Sol Xhigh completed independent technical assessments. Public claims were checked against targeted source and runtime evidence.

Both branches appended a test to keybindings-menu-test.sh. The terminal-bind test from quattro runs first because it needs the stock hyprland.lua, which the plugin-list test overwrites.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@omarchybot omarchybot added the verified Omarchy Triage has verified that this issue is ready for final review label Oct 2, 2026
@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed by Claude Opus 5.5 and run on a worker built from the Omarchy ISO. The fix is correct, and the bug reproduces. No second opinion ran on this one: omabot's daily review budget was spent, so Codex never saw it.

Conflict resolved. This branch conflicted with quattro in test/shell.d/keybindings-menu-test.sh, because #13521 appended a test there too. I pushed 7a9643a, a merge of quattro that keeps both tests. Quattro's terminal-bind test runs first because it needs the stock hyprland.lua, which your test overwrites. Nothing else changed.

What was checked, at 7a9643a:

  • keybindings-menu-test.sh on the worker: 17 passed with the fix. With only the scanner change reverted, your new test fails on the timeout ("scanning a plugin list finishes without hanging").
  • The worker's real Hyprland session, with for _, p in ipairs(hl.get_loaded_plugins()) do end at the top of ~/.config/hypr/hyprland.lua: without the fix, omarchy-menu-keybindings --print was still spinning when the 15s timeout killed it, and rendered nothing. With the fix it finishes immediately and renders all 224 rows, identical to the stock config's output. On the stock config the output is identical with and without the fix.
  • ./test/cli on the worker: one failure, vscode generated theme references current theme file. It fails the same way without this change. 977e0fc made omarchy-theme-set-vscode copy the theme instead of symlinking it, and test/cli:616 still expects a symlink.
  • The shipped Lua under config/ and default/hypr/ only iterates API results inside functions that the scan never calls, so stock configs never hit the hang or the new nil. As already noted above, directly indexing a query result (hl.get_loaded_plugins()[1].name) now errors instead of returning the stub. The scan catches that error, and it is far better than a hung process.

One unrelated thing for the maintainer: on current quattro, the scan of the stock config already stops at default/hypr/qconsole.lua:123 (mon.scale > 0 compares the stub with a number). Nothing after that point in a user's hyprland.lua is scanned. That is why the hang only shows up when the loop comes before it.

This is labelled verified. It is not labelled ready: that waits on a second opinion and a green suite. Next it waits on the maintainer.

@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Medium risk] Fixes infinite loop in keybinding scanner stub.

The PR is not yet safe to merge because direct positional access in a user config can silently remove later bindings from the menu.

Findings

  1. P1 Direct indexing aborts scanning ▶

Summary

The PR makes numeric indexing of the scanner’s no-op API result return nil, terminating ipairs over offline API queries. It also adds a bounded regression test that checks subsequent bindings. The change introduces a separate failure path for configs that directly index a query result and then access its fields.

Reviews (1) · Last reviewed commit: "Merge origin/quattro into fix/keybinding..."

Comment on lines +191 to +192
if type(key) == "number" then
return nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Direct indexing aborts scanning

If a user's Lua config accesses a field on a known-present API result, such as hl.get_workspace_windows(...)[1].title, the scanner now returns nil for [1]. The field access then errors, ending the config scan before later bindings are collected. The menu silently omits those bindings unless DEBUG=1.

Knowledge Base Used: Hyprland configuration and display control

@jandrusk

jandrusk commented Oct 8, 2026

Copy link
Copy Markdown

For triage: this is the generic fix for the "keybindings menu hangs when a config iterates an hl.get_*() result" cluster: #7025 (canonical), #12124, #14243, #14302 and #12852. #14128 is a newer open PR for the same hang, aimed at hl.get_workspaces(). Since this one returns nil for every numeric index, it covers all of the get_* queries, so it would be good to pick one PR and close the issues against it.

The direct-index case greptile flagged (hl.get_loaded_plugins()[1].name now errors and stops later bindings from being discovered) looks like the right trade-off compared with spinning forever. A one-line note in the comment saying so would help the next reader.

Both sides appended a test to test/shell.d/keybindings-menu-test.sh; keep both. Quattro's translation test runs first, since this branch's test overwrites hyprland.lua.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@omarchybot omarchybot added verified Omarchy Triage has verified that this issue is ready for final review ready Good to merge labels Oct 14, 2026
@omarchybot

Copy link
Copy Markdown
Collaborator

This branch conflicted with quattro again in test/shell.d/keybindings-menu-test.sh, because #15326 appended a translation test there. I pushed 1eb6ae7, a merge of quattro that keeps both tests. Quattro's runs first, since yours overwrites hyprland.lua. The scanner change is unchanged.

What was checked, at 1eb6ae7, on a worker built from the Omarchy ISO:

On this worker, quattro's new translation test needs the checkout's bin/ on PATH, because the worker's installed Omarchy predates omarchy-language. It fails the same way on untouched quattro, so it is unrelated to this PR. The runs above put bin/ on PATH.

Second opinion: Codex Medium. It agreed the hang is real and that the fix is proportionate. That agreement came after reading my notes, so its independence is not guaranteed. It also flagged the trade-off already discussed above: hl.get_monitors()[1].name now errors, and the scan loses bindings declared after that line. Any fix that lets ipairs stop has this trade-off. It is better than a process stuck at 100% CPU.

Competing fix: #14128 fixes the same hang for hl.get_*s() results. This PR is older and fixes the same cause, so it stays the one to land. Codex reached the same conclusion after testing both scanners. It found the direct-index trade-off identical under both. #14128's other changes are separate hardening, not part of this fix. They are an instruction-count guard against other runaway loops, __lt/__le stubs so comparisons like hl.get_cursor_pos().x > 0 no longer stop the scan, and clearing the bind maps before a rescan. They are the maintainer's call.

This PR now closes #7025, #12124, #14243 and #14302. I did not link #12852: its report leaves the cause of the runaway unidentified, so I could not show this change fixes it.

This is labelled verified and ready, and now waits on the maintainer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready Good to merge verified Omarchy Triage has verified that this issue is ready for final review

Projects

None yet

4 participants