Skip to content

fix(custom-commands): match whole-word phrases without a dynamic RegExp - #1503

Merged
akramcodez merged 2 commits into
Nano-Collective:mainfrom
addyCooks:fix/semgrep-contains-phrase
Sep 28, 2026
Merged

akramcodez merged 2 commits into
Nano-Collective:mainfrom
addyCooks:fix/semgrep-contains-phrase

Conversation

@addyCooks

Copy link
Copy Markdown
Collaborator

Description

Semgrep's blocking detect-non-literal-regexp rule flags containsPhrase in source/custom-commands/loader.ts, which fails the Semgrep job on every PR branched from main since 4002073 (e.g. #1501). The phrase was already escaped before it reached RegExp, so the finding is a false positive, but the scanner can't see that.

This replaces the RegExp with an indexOf scan that keeps the same behaviour: matching is case-insensitive, and a match must not touch [a-z0-9] on either side, so the tag test still does not match "latest". The finding goes away instead of being hidden with a nosemgrep comment.

The existing whole-word test is extended to cover a later whole-word hit after an embedded one (the latest TEST.).

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Not needed: internal refactor with no user-facing change.

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files (extends the existing whole-word test in loader.spec.ts)
  • All existing tests pass (pnpm test:all completes successfully) (see notes below)
  • Tests cover both success and error scenarios (covers the match and no-match cases; the helper has no error path)

Notes: run on Windows 11 against this commit.

  • test:format, test:types, test:types:vscode, test:lint, test:knip and test:audit pass.
  • Semgrep: the loader.ts finding is gone. Locally it still reports 4 detect-child-process findings (Semgrep 1.175.0) in custom-tools/handler.ts, services/bash-sandbox.ts and tools/git/utils.ts. This PR doesn't touch those files, and CI doesn't report them.
  • AVA: loader.spec.ts and every custom-commands and skills spec pass. The remaining failures are Windows-specific (paths, Unix sockets, POSIX process groups, shell scripts). 94 failures are identical on unmodified main (9b78cdd5). The only difference is one UI test that failed here once and passes 3/3 when re-run alone, while 13 others fail only on main. CI on Linux is the authoritative run.

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Not applicable: this is a pure string helper with no provider or MCP involvement.

Checklist

  • If this was for an open issue, I was assigned to it (N/A, this fixes CI)
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed) (not needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md) (N/A)

Semgrep's blocking detect-non-literal-regexp rule flags containsPhrase in
loader.ts, which fails the Semgrep job on every PR branched from main since
4002073. The phrase was already escaped, so the finding is a false positive,
but the scanner cannot see that.

Replace the RegExp with an indexOf scan that keeps the same behaviour:
case-insensitive, and a match must not touch [a-z0-9] on either side, so the
tag `test` still does not match "latest". Extend the whole-word test to cover
a later whole-word hit after an embedded one.
@github-actions

Copy link
Copy Markdown
Contributor

No changeset found

This PR does not add a changeset, so it will not appear in the changelog or trigger a release.

If the change is user-facing, add one:

pnpm changeset

Pick a bump (patch / minor / major) and write the changelog entry in our usual voice ("Added X... Thanks to @you. Closes #123."), then commit the generated .changeset/*.md file.

If this PR is docs-only or a chore that needs no release note, you can ignore this — or run pnpm changeset --empty to record that intentionally.

@github-actions

Copy link
Copy Markdown
Contributor

nc-review: comments — 1 nit

@addyCooks — a few things worth a look, none blocking.

The PR replaces a dynamic RegExp in containsPhrase with an indexOf scan that preserves the prior whole-word, case-insensitive behaviour and silences a Semgrep false positive. The new implementation correctly handles edge cases including matches at string boundaries (where text[i-1] or text[i + needle.length] is undefined), and the added test case verifies that a whole-word hit after an embedded substring still matches — matching the original regex semantics. No user-facing change, so the omitted changeset is correct.

⚪ nit · completeness · source/custom-commands/loader.ts:22

The new containsPhrase body lowercases the entire haystack once up front (const text = haystack.toLowerCase();) before the loop. This is fine for the typical request string passed in from findRelevantCommands, but worth noting that calculateRelevanceScore already passes requestLower, so the second lowercase pass is redundant on that path. The original regex also only had to lowercase the needle. Keeping this is harmless and arguably clearer; flagging only because it is a small behavioural difference (extra allocation) relative to the previous code.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:comments nc-review left non-blocking findings label Sep 26, 2026
Every caller already passes the request lowercased (requestLower), so
containsPhrase no longer lowercases the haystack a second time. The
parameter is renamed haystackLower and the doc comment states that the
caller supplies it lowercased.
@akramcodez
akramcodez merged commit 79c4f6e into Nano-Collective:main Sep 28, 2026
16 checks passed
@akramcodez

Copy link
Copy Markdown
Member

Thanks for the PR @addyCooks

@addyCooks

Copy link
Copy Markdown
Collaborator Author

Thanks @akramcodez : )

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

Labels

agent:comments nc-review left non-blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants