Skip to content

fix(code-engineer-skill): harden CLI parsing and remove runtime install#360

Open
bgauryy wants to merge 3 commits into
mainfrom
code-review-octocode-mcp
Open

fix(code-engineer-skill): harden CLI parsing and remove runtime install#360
bgauryy wants to merge 3 commits into
mainfrom
code-review-octocode-mcp

Conversation

@bgauryy
Copy link
Copy Markdown
Owner

@bgauryy bgauryy commented Mar 26, 2026

No description provided.

Copy link
Copy Markdown

@JiwaniZakir JiwaniZakir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Math.max(1, ...) fix applied consistently across codeSearch.ts, pullRequestSearch.ts, and repoSearch.ts prevents a zero totalPages when total_count is 0, but it introduces a subtle semantic issue: callers will now receive totalPages: 1 even when the result set is empty, which misrepresents the actual state. Since clampedPage lands on 1 and hasMore is false, the behavior is functionally safe, but a caller inspecting totalPages alone would incorrectly infer there's a page of content. A cleaner fix would be an explicit early guard — e.g., const totalPages = totalMatches === 0 ? 0 : Math.min(Math.ceil(totalMatches / perPage), 10) — keeping totalPages: 0 as the canonical signal for "no results."

On the cache side, ensureCapacityForInsertion in client.ts (and its counterpart ensureProviderCapacityForInsertion in factory.ts) is now called unconditionally before every insertion, whereas the original code only triggered eviction when instances.size >= MAX_INSTANCES. This always incurs a map iteration even when the cache is nearly empty. A simple guard like if (instances.size >= MAX_INSTANCES - 1) would preserve the original lazy-eviction behavior while still fixing the off-by-one growth bug this PR targets.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants