Skip to content

KaStringAsync.KeyEquals branch 1 computes a prefix-only Substring offset under a Contains guard #583

Closed

Description

@drmoisan

Summary

Branch 1 of KaStringAsync.KeyEquals guards on a substring test but computes its Update argument with prefix arithmetic. The two are only consistent when other is a prefix of Key.

QuickFiler/Controllers/KaStringAsync.cs:

  • The branch-1 guard is Key.Contains(other) — a substring test.
  • Its body computes Update(Key.Substring(other.Length - 1, 1)), an offset that is only meaningful when other is a prefix of Key.

For Key = "abc" and other = "b", Contains is true and the expression yields "a", which is neither the matched character nor the character following it.

Reachability

Reachable in principle whenever the registered digit width is 2. GenerateStringKbdAction (QuickFiler/Controllers/QfcCollectionController.cs:1363-1385) registers keys "01" through "12" at that width; typing "1" matches "01" at index 1, as a substring rather than as a prefix.

It has no observable effect today, because Update is null on every KaStringAsync instance production creates:

  • QfcCollectionController.cs:1376-1383 passes null for both update and toggleControl.
  • KbdActions.Add(string, TKey, VDelegate) builds its element with UClass instance = new() (KbdActions.cs:99), the parameterless constructor, which assigns neither callback.

So every Update is not null guard in KeyEquals evaluates false on every production evaluation. This is a latent defect, not a live one.

Why it was excluded from #445

Fixing it correctly requires a design decision that #445 was not scoped to make: whether branch 1 should test StartsWith instead of Contains. That is a keyboard-filtering behaviour change, and the current substring semantics are pinned by an existing test at QuickFiler.Test/Controllers/KbdActionsTests.cs:71-76. Changing it would invert that test rather than leave it passing.

Per the CLAUDE.md Bugfix Workflow, section 2 ("If you uncover deeper design problems, open a new issue instead of widening scope"), the defect was recorded and deferred rather than folded into #445. Issue #445's acceptance criterion AC19 requires this issue to exist, and its P4-T4 gate verifies the expression was left unchanged:

  • Key.Substring(other.Length - 1, 1) in KaStringAsync.cs — count 1, unchanged.
  • Key.Contains(other) in KaStringAsync.cs — count 1, unchanged.
  • The .Be("b") assertion at KaStringAsyncTests.cs (for Key = "abc", other = "ab", the prefix case) — count 1, unchanged.

Options to consider

  1. Change branch 1 to Key.StartsWith(other). Makes the guard and the offset arithmetic consistent. Requires re-deciding, and re-pinning, the keyboard matching semantics currently asserted by KbdActionsTests.cs:71-76.
  2. Keep Contains and correct the offset. For a substring match, compute the index via Key.IndexOf(other) and derive the intended character from that, leaving matching semantics untouched.
  3. Document the prefix precondition and constrain registration so non-prefix matches cannot arise.

Option 2 is the narrowest and does not change which rows match a probe; option 1 is the larger behavioural change. The choice should be made deliberately, with a regression test for the two-digit-width case (Key = "01", other = "1").

Acceptance Criteria

  • A decision is recorded between StartsWith matching, corrected offset arithmetic under Contains, or a documented prefix precondition.
  • KaStringAsync.KeyEquals branch 1 no longer computes a substring offset that is meaningless for a non-prefix match.
  • A regression test covers the two-digit-width non-prefix case (for example Key = "01" with other = "1") with a non-null Update.
  • QuickFiler.Test/Controllers/KbdActionsTests.cs:71-76 either still passes unchanged, or its change is explicitly justified as the intended behavioural change.
  • The full C# toolchain passes: csharpier check ., the analyzer build, the nullable build, and the MSTest suite.

References

  • Parent issue: Bug: quickfiler-keyboard-action-contract-defects #445 (quickfiler-keyboard-action-contract-defects), acceptance criterion AC19.
  • Spec: docs/features/active/2026-08-07-quickfiler-keyboard-action-contract-defects-445/spec.md, "Scope & Non-Goals" and "Rollout & Follow-up".
  • Research: docs/features/active/2026-08-07-quickfiler-keyboard-action-contract-defects-445/research/keyboard-action-contract-defects.2026-08-21T18-20.md.

Activity

  1. added a commit that references this issue on Aug 22, 2026
  2. drmoisan commented on Sep 11, 2026

    @drmoisan
    OwnerAuthor

    Maintainer decision recorded 2026-09-11

    Resolution: Option 2 - keep the Contains guard and correct the offset arithmetic.

    Branch 1 of KaStringAsync.KeyEquals keeps its substring test. The Update argument is computed from the match position (Key.IndexOf(other)) so that Update receives the last matched character for every matching row, not only for prefix matches. For the two-digit registration "01" through "12", typing 1 continues to match 01, 10, 11, and 12; the 01 row now yields 1 instead of 0.

    Rationale: this is the only option with no user-visible change to keyboard filtering, and Update is null on every production instance today, so the correction carries no runtime risk. The pinned test FilterKeys_WhenDistinctStoredKeysCoexist_PreservesKeyboardMatchingSemantics must continue to pass unchanged.

    This issue is scheduled into the consolidated bug parallel run being planned on 2026-09-11.

  3. drmoisan commented on Sep 14, 2026

    @drmoisan
    OwnerAuthor

    Delivered by PR #874, merged as 39ce289 on 2026-09-13.

    KaStringAsync.KeyEquals branch-1 offset arithmetic fixed per the maintainer's recorded Option 2 decision: the Update offset now derives from Key.IndexOf(other, StringComparison.Ordinal) + other.Length - 1 instead of prefix-only arithmetic. The Contains guard is unchanged and no StartsWith was introduced.

    6/6 acceptance criteria. Single clean toolchain pass: CSharpier 0 of 1624 files needing formatting, analyzer and nullable rebuilds both 0 errors, coverage-instrumented MSTest run 1394/1394 passed. KaStringAsync.cs changed-line coverage 100 percent (6/6) with no regression on any previously covered line. The pinned KbdActionsTests file was unmodified and passed 4/4 both before and after.

    Regression case from the issue confirmed: Key="01", other="1", non-null Update now yields "1" where it previously yielded "0".

    Closed manually - the PR carried no closing keyword.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions