Skip to content

fix: off-by-one bounds check in KeyMap.unbind() - #1735

Merged
gnodet merged 3 commits into
masterfrom
fix-keymap-unbind-bounds
Mar 31, 2026
Merged

gnodet merged 3 commits into
masterfrom
fix-keymap-unbind-bounds

Conversation

@gnodet

@gnodet gnodet commented Mar 30, 2026

Copy link
Copy Markdown
Member

Summary

  • Fix off-by-one error in KeyMap.unbind() where bounds checks on lines 631 and 641 used > instead of >=, causing ArrayIndexOutOfBoundsException when a character's value equals mapping.length (128)
  • The bind() method already correctly uses >= at line 672; this aligns unbind() with the same pattern
  • Add test covering the boundary condition

Test plan

  • New testUnbindBoundaryChar test verifies unbinding a key sequence with char value == KEYMAP_LENGTH returns gracefully without throwing
  • All existing reader module tests pass (200 tests, 0 failures)

@augmentcode

augmentcode Bot commented Mar 30, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: Fixes an off-by-one bounds check in KeyMap.unbind() that could throw ArrayIndexOutOfBoundsException for key sequences containing a char equal to mapping.length.

Changes:

  • Updated both bounds checks in KeyMap.unbind(..) from > to >=, matching the existing bind(..) behavior.
  • Added a JUnit test covering unbinding with a boundary char value (128) to ensure it returns gracefully.

Notes: This aligns unbind/bind behavior around the fixed-size keymap table (KEYMAP_LENGTH).

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.


// Unbinding a multi-char sequence where the final char is at the boundary
map.bind("action", "a");
map.unbind("a" + boundaryChar);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

map.unbind("a" + boundaryChar) doesn’t currently exercise the “final char is at the boundary” path: after map.bind("action", "a"), map.mapping['a'] is not a KeyMap, so KeyMap.unbind(..) returns early before checking the last character’s bounds. This makes the test comment a bit misleading and may leave the multi-char-final-char boundary case untested.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

@sonarqubecloud

Copy link
Copy Markdown

@gnodet gnodet added the bug label Mar 31, 2026
@gnodet
gnodet merged commit 46ae4b4 into master Mar 31, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant