fix(search): shift Spotlight file-name highlights by the platform separator

[Vidhaankhare16]

On Windows, a Spotlight file hit that matched on the path underlines the wrong characters of the
file name — or nothing at all.

Defect

GlobalSearchService.searchFiles matches the query against file.relativePath, then rebases the
resulting highlight ranges onto the file name, because SearchResult.FileResult.matchRanges
index the name:

val fileNameStart = file.relativePath.lastIndexOf('/') + 1

IndexedFile.relativePath is derived from two absolute paths by FileIndexer.relativeToRoot, so
on Windows it is joined with \. lastIndexOf('/') returns -1, fileNameStart becomes 0, and the ranges are
shifted by nothing before being clamped to the name's length.

FuzzyMatcher is a greedy subsequence matcher that collapses matched indices into ranges. For the
query uiButton against app\ui\Button.kt it matches indices [4,5,7,8,9,10,11,12], i.e. ranges
[4,6) and [7,13). Against the name Button.kt:

	fileNameStart	ranges kept	underlined
today, Windows	0	[4,6), [7,9)	on, kt
today, POSIX	7	[0,6)	Button
this PR, either	7	[0,6)	Button

A match that lands entirely before the name's length is dropped by the existing filter, so the
other visible symptom is a result with no underline at all — e.g. src\main\Main.kt matching
Main.

Change

One private helper, and the call site that uses it:

private fun fileNameStartIn(relativePath: String): Int =
    maxOf(relativePath.lastIndexOf(File.separatorChar), relativePath.lastIndexOf('/')) + 1

/ is checked alongside File.separatorChar, not instead of it:

• On a POSIX host the two are the same character, so that host's behaviour is bit-for-bit
  unchanged. The third test pins this.
• On Windows, / is not a legal file-name character, so it cannot split a name by accident.
• A bare lastIndexOf('\') on every platform would be wrong: a backslash is a legal POSIX
  file-name character, and it would split dir/we\ird.kt inside its own name.

ContentSearchService, in this package, already reaches for File.separatorChar for the same
reason.

The separator is a parameter defaulting to the host's, and the helper is internal, so tests can
inject it. That is not decoration: because the rule is deliberately platform-dependent, a
Windows-shaped path fed to a POSIX host proves nothing there — the shift is correctly 0 — so
without the parameter the Windows case could only be asserted on a Windows runner. An earlier
revision of this PR kept the helper private and a literal \ in the fixture; that failed
build-test (macos-latest) for exactly this reason, and the correction is described in a comment
below.

2 files, +142/-1 (115 of those are the test file).

Validation

Windows 11, Temurin 17 (as in CI), dev @ cf21742df.

dev moved twice during this work, once by a history rewrite. Only this branch's single commit was
ever replayed, with git rebase --onto origin/dev HEAD~1, and the patch-id was checked to be
unchanged across each replay. The full suite plus ktlint and detekt were re-run on each new base
rather than carried over — which mattered, since merging #644 moved the totals from 5239/554 to
5272/559.

• The tests were shown to FAIL first, by mutating fileNameStartIn's body back to the
  pre-fix relativePath.lastIndexOf('/') + 1 rather than reverting the file — the shift cases name
  the helper, so a straight revert would not compile. Result: BUILD FAILED, 5 tests, 2 failures,
  0 errors, 0 skipped, failing for the defect and not a wiring problem:
  a windows path shifts past its last backslash → expected: <7> but was: <0>, and the
  end-to-end case → underlined text ==> expected: <[Button]> but was: <[on, kt]>.
• Green on the fixed tree: GlobalSearchFileHighlightTest 5 tests, 0 failures, 0 skipped.
• Full :composeApp:desktopTest: 5272 tests in 559 classes, 17 skipped, 2 failures. Both
  failures are this machine's known Windows symlink-privilege limitation, not regressions: they
  fail with java.nio.file.FileSystemException: ... A required privilege is not held by the client
  while creating a symlink, in DesktopFileScannerTest and DiscardDownloadContainmentTest.
  Neither class is touched by this change.
• ktlintCheck and detekt: BUILD SUCCESSFUL, exit 0. :composeApp:detekt executed (not
  skipped) with an empty checkstyle report and a 0-byte detekt.txt; ktlintCheck clean.

What each test can honestly prove, since it is not uniform:

• The four shift cases inject the separator and assert identically on every runner. One of them
  pins that a backslash inside a POSIX file name (dir/we\ird.kt) is not mistaken for a
  separator — the regression a blanket lastIndexOf('\') would have caused.
• The one end-to-end case drives the real GlobalSearchService.search(...) with the host's
  separator. It is a red test on Windows and a regression guard elsewhere. Its KDoc says so.

Under the mutation above, 2 of the 5 go red and 3 pass. The 3 that pass cannot distinguish the two
implementations and are guards, not teeth; they are not offered as evidence of the fix.

Not done, stated rather than left silent: no app was launched and the Spotlight dialog was not
opened, so the rendered underline was not inspected visually. The evidence is the ranges returned by
the real GlobalSearchService.search(...), which is the value SearchResultsList draws from.

Overlap

A paginated sweep (gh api .../files --paginate) of all 97 open PRs, with every hit then confirmed
by a local diff against a re-derived merge base rather than trusting the API file list:

• fix: dispatch supported Spotlight commands #702 is the only small, focused PR touching this file. Its entire change here is 2 lines in
  searchCommands (~line 488) — a different function about 200 lines away. No conflict.
• ⬆️ deps:(deps): bump kotlin from 2.4.10 to 2.4.20 #654 / deps: align Netty 4.2.18 and verify native IPC transports #657 (dependabot) show a GlobalSearchService.kt diff only because their merge base
  (8148bec2) predates the searchTabs URL/file-path work that has since landed on dev
  independently. Stale-base noise, not an edit.
• docs: add automated security audit workflow template #613 / fix/global-search-tab-url-path #607 / feat(browser): report browser calls still in flight, so teardown can see them (#300) #601 / fix(repository): a cancelled health check is not a store outage #599 / feat(mcp): causal context binding & stale execution fencing for gover… #597 / fix(run): preserve platform-explicit artifact paths and coverage #595 are 209-214 file PRs bulk-drafted at one timestamp;
  same stale-base noise.

Unrelated to my own #644, which touched FileIndexer.kt and not this file. #644 has since merged
(cf21742df), and this branch is rebased directly onto it. It changed how relativePath is built
— root.absolutePath.relativize(child…).toString() at FileIndexer.relativeToRoot — but not
what it is joined with, which is still \ on Windows. So this defect is identical before and after
it, and neither PR fixes the other's.

No merge or auto-merge is authorized.

[Vidhaankhare16]

The macOS job failed, and it is my error, not a flake. Correcting the record before I push a fix.

What failed and why

build-test (macos-latest) — GlobalSearchFileHighlightTest, 2 of 3 cases, at lines 67 and 78.
5238 tests completed, 2 failed. The ubuntu and windows jobs were cancelled by fail-fast, so they
are not independent evidence either way.

The cause is a flaw in the test, not in the fix. fileNameStartIn is deliberately
platform-dependent — it looks for File.separatorChar. On a macOS runner that is /, so the
backslash-joined fixture app\ui\Button.kt has no separator to find, the shift is correctly 0,
and the assertion that the underline is Button fails. The production behaviour is right on both
platforms; the test asserted a Windows-only outcome on every platform.

The claim I got wrong

The PR body said:

The tests do not depend on the host OS. The separator is written into the fixture as a literal
rather than read from File.separatorChar, so the Windows case is asserted identically on a Linux
runner instead of silently passing there.

That is wrong, and the second half is backwards. Writing the separator into the fixture does not
make the assertion host-independent when the function under test reads the host's separator — it
makes it fail everywhere except Windows. I had validated only on Windows, where it passes, and I
should have caught that the two facts could not both be true.

The fix

fileNameStartIn takes the separator as a parameter defaulting to File.separatorChar. Production
is unchanged in behaviour; tests inject the separator, which is what actually makes the Windows case
assertable on any runner:

internal fun fileNameStartIn(
    relativePath: String,
    separator: Char = File.separatorChar,
): Int = maxOf(relativePath.lastIndexOf(separator), relativePath.lastIndexOf('/')) + 1

The tests are now split by what each can honestly prove:

• Four shift cases inject the separator and assert the same thing on every OS, including that a
  backslash inside a POSIX file name (dir/we\ird.kt) is not mistaken for a separator.
• One end-to-end case drives the real GlobalSearchService.search(...) using the host's
  separator. That is a red test on Windows and a regression guard elsewhere — stated in its KDoc so
  the asymmetry is on purpose rather than by accident.

This also widens fileNameStartIn from private to internal, which the previous PR body
specifically claimed it had avoided. That claim no longer holds, and the tradeoff is deliberate:
proving the Windows rule on a Linux runner is worth one internal in this file.

Teeth re-checked by mutating the helper body back to the pre-fix lastIndexOf('/') + 1 rather than
by deleting it, since the shift cases reference it by name and a straight revert would not compile.

No merge or auto-merge is authorized.

[kshivang]

@claude Please review current head 8f69209 against dev for correctness, platform separator behavior and regression test coverage. This is the first review request for this PR in the current sweep.

[claude]

Claude finished @kshivang's task in 3m 48s —— View job

---

Review of 8f69209 against dev

• Read the PR diff and the surrounding searchFiles code
• Verify the defect (how IndexedFile.relativePath is built)
• Verify the fix's platform-separator behavior, including edge cases
• Trace the claimed uiButton / app\ui\Button.kt ranges through FuzzyMatcher
• Assess regression test coverage and what each test can prove
• Check repo conventions (commonMain java.io.*, import order, detekt baseline, no em-dashes)

Verdict

The defect is real, the fix is correct and minimal, and the four shift tests have genuine teeth. I found no correctness problem in the change. Notes below are all optional/nits.

---

Correctness - confirmed

The defect is genuine. FileIndexer.relativeToRoot (FileIndexer.kt:288) is root.absolutePath.relativize(child.toPath().toAbsolutePath()).toString(), and Path.toString() joins with the platform separator - so relativePath really is \-joined on Windows, and the old lastIndexOf('/') really did return -1. The consumer side checks out too: GlobalSearchDialog.kt:1394 renders highlightMatches(result.name, result.matchRanges, …), so the ranges do index the name, which is what makes the shift necessary.

The arithmetic checks out. For uiButton against app\ui\Button.kt (lowered), FuzzyMatcher.match is a leftmost-greedy subsequence pass (FuzzyMatcher.kt:51-58), matching indices [4,5,7,8,9,10,11,12] → ranges [4,6) and [7,13). With fileNameStart = 7, [4,6) is dropped by the existing it.start >= fileNameStart || it.end > fileNameStart filter and [7,13) rebases to [0,6) = Button. With the pre-fix 0 it kept both and clamped to [4,6)="on" and [7,9)="kt". Both tables in the PR body match what the code does.

The name branch cannot mask this. The path branch is only reached when the query is not a subsequence of the name, so any hit there necessarily consumes directory characters - which is exactly the case the shift exists for. Nothing shadows the fix.

Edge cases I checked, all fine:

• maxOf(lastIndexOf(separator), lastIndexOf('/')) picks the rightmost of the two, so a mixed-separator path like app/ui\Button.kt resolves to 7 on Windows rather than to the earlier /. That is a genuine bonus over indexOf-style alternatives.
• A range straddling the separator maps correctly: start = maxOf(0, start - fileNameStart) clamps the directory half away while end - fileNameStart keeps the name half (unreachable in practice, since a query character would have to match the separator itself, but it degrades correctly).
• relativeToRoot's IllegalArgumentException fallback returns a bare child.name, where the helper correctly returns 0.
• java.io.File in commonMain is fine here: composeApp declares only jvm("desktop") (build.gradle.kts:900), and FileIndexer.kt / ContentSearchService.kt in this same package already import it. Import placement (others → java.** → kotlin.**) matches ktlint's default layout in both files.
• TooManyFunctions:GlobalSearchService.kt$GlobalSearchService is already in composeApp/detekt-baseline.xml:1449, so the added function does not need a baseline edit.
• No U+2014 in either file; KDoc uses the spaced hyphen the repo requires.

Platform separator behavior - agreed with the reasoning

Checking / alongside File.separatorChar rather than instead of it is the right call, and the KDoc's justification is the one I'd give: POSIX is bit-for-bit unchanged (same character), / is illegal in a Windows file name so it cannot split one by accident, and a blanket lastIndexOf('\\') would split dir/we\ird.kt inside its own name. Test 4 pins that last one with real teeth - mutating the body to maxOf(lastIndexOf('\\'), lastIndexOf('/')) returns 7 there and goes red.

Worth recording that the repo now carries two conventions for this question: UpdatePathValidator.fileNameComponent (UpdatePathValidator.kt:30) and FileEventBus.kt:71 both use the blanket substringAfterLast('/').substringAfterLast('\\'). Those produce a display name, where a wrongly split POSIX name is cosmetic; here the value is a substring offset driving an underline, so the stricter rule is justified. Not something to change in this PR - but if a third call site appears, the divergence is worth a shared helper with this file's reasoning attached.

Regression coverage - honest, with one residual gap

The split the author describes is accurate, and I agree with the self-assessment that only 2 of the 5 have teeth:

Test	Runs on	Fails under the pre-fix body
shift: windows backslash	every runner	yes (7 vs 0)
shift: posix slash	every runner	no (guard)
shift: bare name	every runner	no (guard)
shift: backslash inside a POSIX name	every runner	no for the '/'-only mutation, yes for the blanket-\\ one
end-to-end search(...)	every runner, host separator	Windows only

The residual gap: searchFiles calls fileNameStartIn(file.relativePath) with the default separator, so the rebase + clamp + filter pipeline - which is where the observable "underlines on, kt" and "underlines nothing" symptoms actually come from - is only exercised with Windows shapes on a Windows runner. The unit tests prove the index, the end-to-end test proves the mapping only on the host's separator. If you want that closed on every runner without threading a separator through searchFiles, the cheap version is to extract the existing filter/map/filter block into an internal fun rebaseOntoName(ranges, fileNameStart, nameLength) and drive it directly with fileNameStart = 7, nameLength = 9, ranges = [[4,6), [7,13)]. That would also make the "range past the name's length underlines nothing" symptom assertable, which nothing currently asserts. Optional. Fix this →)%20so%20the%20rebase%20is%20asserted%20on%20every%20runner%2C%20including%20the%20case%20where%20every%20range%20lands%20past%20the%20name%27s%20length%20and%20nothing%20is%20underlined&repo=risa-labs-inc/BossConsole)

Test hygiene is otherwise good: SearchSources.clearForTests() in @BeforeTest/@AfterTest matches GlobalSearchOrderingTest, Char(92) sidesteps escape-rewriting tooling, and filterIsInstance<SearchResult.FileResult>().single() cannot pass vacuously (an empty list throws).

Nits (non-blocking)

1. One claim in the PR body is not reachable. The body offers src\main\Main.kt matching Main as the "no underline at all" symptom. Main is a subsequence of Main.kt, so the name branch (GlobalSearchService.kt:281-294) returns name-relative ranges and never reaches the path branch. The generic claim in the commit message ("a match landing past the name's length was dropped and underlined nothing") is reachable - a short name under a long directory prefix - so only the example is wrong, not the symptom. Worth correcting in the body so the next reader does not chase it.
2. it.start >= fileNameStart is redundant in the pre-existing filter: ranges always have end > start, so the second disjunct subsumes the first. Pre-existing, untouched by this PR, mentioned only because the line is in the diff context.
3. Inconsistent backslash escaping inside the KDoc: line 247 writes `\` and line 254 writes `lastIndexOf('\\')`. Both render, but one prose block spelling the same character two ways is a small speed bump.
4. Considered alternative, for the record: because IndexedFile already carries name, fileNameStart could be relativePath.length - name.length when relativePath.endsWith(name) - separator-agnostic, no platform rule and no injected parameter at all. I am not recommending the swap: it silently depends on name being a suffix of relativePath (true today for both relativeToRoot branches, but an invariant nothing pins), whereas the current helper is self-contained and testable in isolation. Noting it so the trade is visible rather than re-litigated later.

What I could not verify

No Gradle cache exists on this runner (~/.gradle/caches is absent), so I did not compile or run :composeApp:desktopTest, ktlintCheck or detekt - this review is static plus the author's reported Windows run. CI on this PR is the authority for the green/red claims, and the macOS failure the author already corrected is exactly the class of thing static review would miss. I did not launch the app or look at the rendered underline either.
· branch fix/search-highlight-windows-separator

[kshivang]

@Vidhaankhare16 Claude confirmed the platform-separator fix is correct and the regression tests have real teeth; exact-head CI was green across the full matrix. I squash-merged 8f69209995298b495b6881b46ec7fbc38f0c1e77 into dev as da23ea78133d8d0a2a77699a84ad8a85af5d4f13. Original-work score: 9/10 (impact 2/3, correctness 3/3, completeness 2/2, maintainability 2/2). No agent source repair was needed.