fix: address Copilot review follow-ups from #56; docs: add n8n integration guide#58
Merged
Merged
Conversation
…ation guide Addresses all four review comments left on #56 by copilot-pull-request-reviewer: - knownToolIDs (an ever-growing Set) replaced with slotToolIDs, a Map keyed by slot name whose entries are replaced (not accumulated) on each reuse. Bounded by pool size regardless of how many requests/unique tool schemas a long-lived process handles over its lifetime, instead of growing forever and bloating every subsequent turn's tools payload. - registerToolBridge() now releases its acquired slot back to the pool if anything after acquisition throws (e.g. client.mcp.add() failing to spawn/register the bridge). Previously such a slot was lost forever, since runAgentTurn() only releases a bridge once registerToolBridge() has already returned successfully - repeated failures would eventually exhaust the whole pool and hang every future tool-calling request in acquireBridgeSlot(). - index.test.js: the slot-isolation test now releases both bridges it acquires via releaseToolBridge() (now exported) in an after() hook, instead of leaking them into the shared global pool for the rest of the test process. - New regression test for the slot-leak-on-failure fix: repeats a failing registration well beyond the pool size and confirms a subsequent registration still succeeds promptly (wrapped in a timeout race, since node:test has no default per-test timeout and a regression would otherwise hang the suite instead of failing it). README.md: documents using the proxy to pipe OpenCode's authenticated providers (GitHub Copilot, Anthropic, Bedrock, local Ollama, etc.) into n8n's native AI nodes - credential setup for both OpenAI/Anthropic node types, the model-picker auto-populating from GET /v1/models, Basic LLM Chain vs AI Agent + Tools, and the Docker-on-a-different-host + firewall caveat. Testing: - npm test - 141 passed (140 + 1 new) - npm run lint - clean
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.
Summary
Addresses all four review comments left by
copilot-pull-request-revieweron #56, plus documents the n8n integration use case in the README.Review follow-ups
knownToolIDsgrows unbounded (real issue): replaced the ever-growingSetwithslotToolIDs, aMapkeyed by slot name whose entries are replaced (not accumulated) every time that slot is reused. Bounded by pool size regardless of how many requests / unique tool schemas a long-lived process handles over its lifetime, instead of growing forever and bloating every subsequent turn'stoolspayload.registerToolBridge()throws after acquiring a slot (real issue, more serious): now releases its acquired slot back to the pool if anything after acquisition throws (e.g.client.mcp.add()failing to spawn/register the bridge). Previously such a slot was lost forever, sincerunAgentTurn()only releases a bridge onceregisterToolBridge()has already returned successfully — repeated failures would eventually exhaust the whole pool and hang every future tool-calling request inacquireBridgeSlot().releaseToolBridge()(now exported) in anafter()hook instead of leaking them into the shared global pool for the rest of the test process.buildToolsMap(baseTools, null)case doesn't actually touch global state at all in this implementation (it returns early before callinggetToolBridgeState()).Also added a new regression test specifically for #2 (the slot-leak-on-failure fix): repeats a failing registration well beyond the pool size and confirms a subsequent registration still succeeds promptly, wrapped in a timeout race since
node:testhas no default per-test timeout and a regression would otherwise hang the suite instead of failing it cleanly.Docs
README.md now documents using the proxy to pipe OpenCode's authenticated providers (GitHub Copilot, Anthropic, Bedrock, local Ollama, etc.) into n8n's native AI nodes — credential setup for both OpenAI/Anthropic node types, the model picker auto-populating from
GET /v1/models, Basic LLM Chain vs. AI Agent + Tools, and the Docker-on-a-different-host + firewall caveat (learned the hard way testing this for real).Testing
npm test— 141 passed (140 + 1 new)npm run lint— cleanRelated