Fix regex API term extraction discarding quotes - #152
Merged
Conversation
longestLiteralSequence() now accumulates " as a valid literal character instead of breaking the sequence on it. extractApiTerm() escapes and wraps the extracted term in an outer pair of quotes (GitHub's documented escaping syntax) whenever it contains a quote, instead of sending an unwrapped term that GitHub would otherwise strip the quotes from or treat too broadly. Verified against the live GitHub API: /"react":\s*"[~^]?[0-9]/ now sends "\"react\"" (227 candidates) instead of the previous bare react term (20288 candidates, mostly noise never surfacing the right files within the API's 1000-result best-match cap). Closes #147
|
Coverage after merging fix/regex-api-term-quotes into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
shouze
changed the title
CodeQL flagged escapeApiTerm() for incomplete string escaping: it escaped double-quote characters but not backslashes, per GitHub's documented escape syntax (which only recognises \\ and \" as escapes). A literal backslash in the term was left unescaped, which could be misread once wrapped in the outer quoted phrase. Escape backslashes first, then quotes, matching standard escaping order and GitHub's own documented rules. longestLiteralSequence() never actually emits a raw backslash in practice (it only ever appends the character *after* a backslash, never the backslash itself), so this is a pure defense-in-depth hardening fix with no observable behaviour change for any existing input -- confirmed by the full test suite staying green. escapeApiTerm() is now exported for direct unit testing of the backslash-then-quote escaping order. See #152 (comment)
|
Coverage after merging fix/regex-api-term-quotes into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes regex-query API term extraction so patterns containing double quotes (e.g. JSON keys like "react") keep those quotes in the derived GitHub Search API term, preventing overly broad candidate sets and the 1000-result cap from hiding relevant matches.
Changes:
- Add
escapeApiTerm()to escape internal quotes/backslashes and wrap extracted terms in GitHub’s exact-phrase quoting syntax when needed. - Update
extractApiTerm()/longestLiteralSequence()to treat"as a literal character and to applyescapeApiTerm()to extracted terms (including OR-joined alternation branches). - Add regression/unit tests covering the reported reproduction pattern and the escaping behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/regex.ts |
Keeps " in extracted literal sequences and escapes/wraps API terms so quoted literals survive GitHub’s query parsing. |
src/regex.test.ts |
Adds regression coverage for issue #147 and direct unit tests for the new escaping behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…quotes Addresses review feedback: the test name implied the escaped output was just \"react\", but escapeApiTerm() also wraps the term in outer quotes, so the actual expectation is "\"react\"". No assertion or behaviour change. See #152 (comment)
|
Coverage after merging fix/regex-api-term-quotes into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Closes #147.
Regex queries like
/"react":\s*"[~^]?[0-9]/were losing almost all their expected results, because the term sent to the GitHub search API dropped every double quote character in the pattern.longestLiteralSequence()insrc/regex.tsnow treats"as a valid literal character instead of a sequence-breaking one, andextractApiTerm()escapes and wraps the extracted term in GitHub's documented quote-escaping syntax whenever it contains a quote.Verified directly against the live GitHub API (org
fulll): the regex above now sends the API term"\"react\""(227 candidates, all containing the literal substring), instead of the previous bare termreact(20288 candidates, mostly unrelated noise such as react-native imports, docs, and@types/react, which never surfaced the rightpackage.jsonfiles within the API's 1000-result best-match cap).Existing behaviour for patterns without quotes (
TODO|FIXME|HACK,from.*['"]axios, etc.) is unchanged.How did you verify your code works?
src/regex.test.tscovering the exact reported pattern and the escapedapiQueryit must now produce.bun test), lint (bun run lint), format check (bun run format:check) and knip (bun run knip); all green.bun run build.ts) and ran the original repro command end to end: the compiled binary with the same query and org now returns 9 repositories instead of 2.