Repository navigation
Fix #5537: Fix capCodePoints returning nearly the full string when maxCodePoints is zero or negative - #5538
Conversation
…xCodePoints is zero or negative
|
Two small things, one of them semantic. The
It would also be great to pin the boundary with a regression test (cap |
…files, add regression tests - capCodePoints now returns '' when maxCodePoints <= 0, matching the contract 'at most maxCodePoints code points' and sibling helpers truncateUtf16Safe/sanitizeUnicodeText (#5537) - Remove empty fork-error.txt and fork-view-error.txt (accidental commits) - Add regression tests for cap 0, -2, and empty string input
|
Thanks for the review, @ggbdpq! Here's what I addressed in the follow-up commit (
|
|
Confirmed on |
…eep capCodePoints fix
|
Resolved merge conflict with
|
hqhq1025
left a comment
There was a problem hiding this comment.
Current-head review found one blocking build regression. The original zero/negative cap guard behaves correctly in isolation, but the conflict resolution revives a retired search implementation and its obsolete test after the project replaced that path with Recall. The exact base builds successfully; this head does not. No schema or migration changes are involved.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
The PR claims to be a one-line fix to an existing capCodePoints helper (#5537), but on main that helper and its entire module do not exist — the diff actually adds a 604-line packages/core/src/thread-search.ts search engine plus a 632-line test, vendored wholesale from an unmerged lane (its own header references "PR-SEARCH-2" and a lane greenlight). The one-line guard itself is plausible, but it ships inside dead code with no production caller, and the PR body describes a different patch than the one committed. The issue is real only in the unmerged feature lane, not in the base branch.
Findings
- [P1] Premise false on main —
capCodePoints,runThreadSearch, andSNIPPET_MAX_CODE_POINTShave zero matches in main's source (only in gitignored staledist/output;.gitignore:2).packages/core/src/thread-search.tsdoes not exist onmain(main instead has the 69-linepackages/core/src/transcript-search.tsprimitives). Framed as a fix, this PR actually merges an entire unreviewed search module;runThreadSearchhas no caller anywhere except the new test (worktree grep: onlythread-search.ts:140and the test file). - [P1] Test cannot compile or run —
apps/desktop/src/main/__tests__/thread-search.test.ts:31imports@maka/core/thread-search, butpackages/core/package.jsonhas no"./thread-search"export (neither main nor the PR adds one). WithmoduleResolution: "Bundler"(apps/desktop/tsconfig.main.json,tsconfig.base.json) this is a TS2307 at desktop main compile; at runtimetest:dist(apps/desktop/package.json:47,node --test "dist/main/**/*.test.js") would hit ERR_PACKAGE_PATH_NOT_EXPORTED. CI never ran: "no checks reported on the 'pamod-madubashana/issue-5537-35505003700' branch". - [P2] PR body contradicts the shipped code — the body states the guard is
return codePoints.length === 0 ? value : '…'and its verification log claims"hello" @0 => "…"; the diff shipsif (maxCodePoints <= 0) return '';(packages/core/src/thread-search.ts:602) and the regression test asserts''. The body's cited parity argument also backfires: the siblingsanitizeUnicodeText(packages/core/src/text-sanitize.ts:101) returns'…', not'', for non-empty input at cap 0. Which contract is intended is unresolved. - [P2] No reachable trigger — the only in-module callers pass
SNIPPET_MAX_CODE_POINTS = 240(thread-search.ts:268,326), so the zero/negative-cap path is unreachable in production even after merge; the fix belongs in the unmerged PR-SEARCH-2 lane where the module is actually being introduced, not as a standalone module drop on main.
Verdict
needs-changes — the bug doesn't exist on main, the new test's import is unresolvable (@maka/core/thread-search not exported), and the PR body describes a different fix than the diff ships.
Fixes #5537
PR Summary — Fix
capCodePointsfor zero/negative caps (#5537)What changed
packages/core/src/thread-search.ts—capCodePointsnow handlesmaxCodePoints <= 0explicitly:slice(0, maxCodePoints - 1)truncation. Non-empty input with a zero or negative cap now returns just"…", and empty input still returns""(previously""with a negative cap fell through to the slice and incorrectly returned"…").Why it addresses the issue
codePoints.slice(0, maxCodePoints - 1)used-1as the end index for a 0 cap, and a negative end counts back from the array end — socapCodePoints("hello", 0)returned"hell…"(5 code points) instead of"…", completely bypassing the documented "at mostmaxCodePointscode points" bound. Negative caps behaved the same way.sanitizeUnicodeText. Positive-cap behavior is untouched (stillslice(0, max - 1) + '…'), and current production callers passingSNIPPET_MAX_CODE_POINTS = 240are unaffected.Verification
node_modulesabsent), so the@maka/corebuild fails witherror TS2688: Cannot find type definition file for 'node', and thethread-searchtests (which run from built output vianode --test dist/main/**/*.test.js) cannot execute.capCodePointsfunction was extracted verbatim frompackages/core/src/thread-search.tsand evaluated directly withnode -e, covering the issue's repro plus edge cases:"hello" @0 => "…" (1pt)(was"hell…" (5pts)before the fix)"0123456789…"(40 chars) @0 => "…" (1pt)(was 40pts before the fix)"hello" @-2 => "…" (1pt)(was"he…" (3pts)before the fix)"" @0 => "","" @-2 => ""(empty input stays empty)"hello" @1 => "…","hello" @5 => "hello","hello" @10 => "hello"(positive/no-truncation paths unchanged)