Skip to content

Add Node.js low-level tool-definition E2E test [3/6]#1725

Merged
edburns merged 2 commits into
mainfrom
edburns/1682-03-nodejs-new-test
Jun 18, 2026
Merged

Add Node.js low-level tool-definition E2E test [3/6]#1725
edburns merged 2 commits into
mainfrom
edburns/1682-03-nodejs-new-test

Conversation

@edburns

@edburns edburns commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds Node.js low-level tool-definition E2E coverage and includes the session-lifecycle replay mismatch fix needed for stable cross-platform replay matching.

This PR is related to issue #1682 but does not fix #1682.

What changed

  • Adds low_level_tool_definition test coverage in:
    • nodejs/test/e2e/tools.e2e.test.ts
  • Includes the session-lifecycle fix from the source branch:
    • nodejs/test/e2e/session_lifecycle.e2e.test.ts
    • test/snapshots/session_lifecycle/should_list_created_sessions_after_sending_a_message.yaml
  • Reuses the shared snapshot from PR Add Java low-level tool definition E2E test and skill [1/6] #1721:
    • test/snapshots/tools/low_level_tool_definition.yaml
  • Keeps low-level test tool definitions aligned with what the shared snapshot actually exercises (no unexercised grep override in this scenario).

Dependency / sequencing

Related

Related to issue #1682 but does not fix #1682.

Align low_level_tool_definition coverage with PR #1721 snapshot behavior by only defining tools exercised by the shared snapshot.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@edburns edburns requested a review from a team as a code owner June 18, 2026 19:57
Copilot AI review requested due to automatic review settings June 18, 2026 19:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds Node.js E2E coverage for the shared “low-level tool definition” snapshot scenario, and updates the session-lifecycle replay snapshot + Node test prompt to address a replay-matching mismatch.

Changes:

  • Add Node.js E2E test low_level_tool_definition that exercises explicit tool definitions + available-tools filtering using the shared replay snapshot.
  • Update the session-lifecycle E2E prompt from “Say world” → “Say hi” and adjust the stored replay snapshot content accordingly.
  • Import and use ToolSet in Node E2E tools tests to express source-qualified available-tools patterns.
Show a summary per file
File Description
nodejs/test/e2e/tools.e2e.test.ts Adds the Node E2E test that replays tools/low_level_tool_definition.yaml and validates tool execution + handler side effects.
nodejs/test/e2e/session_lifecycle.e2e.test.ts Changes the second session activity prompt to “Say hi” to match the updated shared replay snapshot.
test/snapshots/session_lifecycle/should_list_created_sessions_after_sending_a_message.yaml Updates the recorded replay conversation for the session-lifecycle scenario (prompt + assistant response).

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment on lines +15 to +18
- role: user
content: Say world
content: Say hi
- role: assistant
content: world
content: Hi! I'm GitHub Copilot CLI, ready to help with your software engineering tasks. What would you like to work on?
Comment on lines +33 to +35
// Sessions must have activity to be persisted to disk
await session1.sendAndWait({ prompt: "Say hello" });
await session2.sendAndWait({ prompt: "Say world" });
await session2.sendAndWait({ prompt: "Say hi" });
- Apply Prettier formatting to tools.e2e.test.ts so Node ubuntu format check passes.

- Drop session lifecycle carryover from this PR by restoring Node session lifecycle files to upstream/main content, keeping this PR focused on low-level tool-definition coverage.

Related to issue #1682 but does not fix #1682.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review

Summary

This PR is part of a planned 6-PR series adding low_level_tool_definition E2E coverage across all SDKs. The Node.js implementation is well-structured and the ToolSet API usage (new ToolSet().addCustom("*").addBuiltIn("web_fetch")) is consistent with the Java implementation added in #1721. ✅

One Consistency Observation: Java vs Node.js Test Scope

The Java test (LowLevelToolDefinitionIT.java) registers three tools in the session config — including a grep override via ToolDefinition.createOverride — while this Node.js test registers only two tools. Both tests replay the same snapshot (test/snapshots/tools/low_level_tool_definition.yaml), which only exercises the first two tools.

The PR description explicitly notes this choice: "no unexercised grep override in this scenario." That reasoning is sound for Node.js — defining tools that the LLM never calls doesn't add replay value here. However, it creates a minor divergence between the two test implementations:

SDK Tools defined Covers override-tool coexistence?
Java 3 (incl. grep override) ✅ incidentally
Node.js (this PR) 2

Suggestion for the remaining PRs in this series (Python, Go, .NET, Rust): consider aligning on one approach — either consistently include the grep override tool to test that override tools can coexist in a session alongside regular tools, or consistently omit it across all SDKs. The Java test covers override-tool coexistence incidentally; the Node.js test relies on its separate overrides built-in tool with custom tool test for that coverage. Either way, it would be good for the series to converge on a single pattern so the low_level_tool_definition test means the same thing in every SDK.

Everything Else Looks Good

Generated by SDK Consistency Review Agent for issue #1725 · sonnet46 1.7M ·

@edburns edburns merged commit cc7e5f3 into main Jun 18, 2026
18 checks passed
@edburns edburns deleted the edburns/1682-03-nodejs-new-test branch June 18, 2026 20:23
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.

Java: Ergonomics: Defining tools

2 participants