fix(tools): coerce stringified numeric IDs in tool input schemas - #103
Conversation
Some MCP transport / model paths deliver `message_id`, `user_id`,
`reply_to`, `offset_id` as JSON strings ("2081") rather than numbers
(2081). The schemas used `z.number()`, which rejects those with
`expected number, received string` β manifesting as `react`,
`pin_message`, `get_member_info`, `download_media`, etc. failing
with InputValidationError even when the model formatted the call
correctly for a number-typed JSON Schema field.
Switch every ID-shaped field to `z.coerce.number().int()`:
- accepts numbers as-is
- coerces numeric strings ("2081" β 2081)
- rejects non-numeric strings ("abc" β NaN β fails .int())
- rejects non-integers (1.5 β fails .int())
Audit test (`tool-id-coercion.test.ts`) walks every tool schema
and fails if any field whose name is in {message_id, user_id,
reply_to, offset_id} still uses raw `z.number()`. Caught one I
missed on the first pass (`download_media`).
1424/1424 tests passing, tsc clean, lint unchanged (9 pre-existing
warnings, 0 errors).
β¦ wiring
Two new layers of coverage in src/__tests__/tool-functional.test.ts:
Part A β Tool definition β bridge call (29 cases)
For each tool, parse representative params through the zod schema,
call tool.execute(parsed, mockBridge), and assert the bridge was
called with the right action name and shape.
- Single-action telegram tools: react, edit_message, delete_message,
forward_message, pin_message, unpin_message, stop_poll,
get_member_info, get_message_by_id, download_media.
- send tool dispatches: text / text-with-reply / text-with-buttons /
delayed text β schedule_message / photo / file / video / voice /
audio / animation / sticker / poll / pollβquiz / location /
contact / dice β 16 dispatch cases proving the typeβbridge map.
- End-to-end coercion: stringified message_id / reply_to / user_id
must arrive at the bridge as numbers after passing through the
schema layer.
Part B β Bridge β Telegram Bot API (15 cases)
createTelegramActionHandler is invoked with synthesized action
bodies and a stubbed bot.api / gateway. Asserts the right
bot.api.* method is called with the right arguments.
- react: standard call, stringified-fallback safety, fallback to π
when custom emoji is rejected by Telegram.
- delete_message, pin_message, unpin_message (with + without id).
- forward_message: in-chat happy path AND cross-chat refusal.
- copy_message.
- edit_message: HTML-rendered call shape AND TELEGRAM_MAX_TEXT
refusal at 5000 chars.
- send_chat_action.
- send_message: full chain incl. gateway.incrementMessages and
reply_parameters.
- schedule_message + cancel_scheduled lifecycle (no real timer fires).
These catch the failure modes plain schema tests miss: action-name
typos between tool and handler, multiplexed dispatch wiring,
post-schema coercion behaviour, and silent grammy API arg drift.
1468/1468 tests passing (was 1424 before this commit, +44 here).
tsc clean, lint unchanged (9 pre-existing warnings, 0 errors).
There was a problem hiding this comment.
Pull request overview
This PR addresses MCP tool input validation failures caused by some transport/model paths delivering numeric ID parameters as JSON strings, by updating tool Zod schemas to coerce ID-shaped fields into integers and adding regression tests to prevent regressions.
Changes:
- Updated tool input schemas to use integer coercion for
message_id,user_id,reply_to, andoffset_id. - Added a regression test suite to verify ID coercion behavior and audit all tool schemas for remaining raw ID
z.number()usage. - Added a functional test suite covering toolβbridge wiring and Telegram action handlerβgrammy API calls.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/tools/messaging.ts | Coerces reply_to and multiple message_id fields to integers to avoid schema rejection when IDs arrive as strings. |
| src/core/tools/history.ts | Coerces offset_id / message_id to integers for history tools, including download_media. |
| src/core/tools/members.ts | Coerces user_id to integer for member lookup tool. |
| src/core/tools/stickers.ts | Coerces user_id to integer for sticker pack management tools. |
| src/tests/tool-id-coercion.test.ts | Adds regression + audit tests for ID coercion across all tools. |
| src/tests/tool-functional.test.ts | Adds broad functional coverage for tool execution wiring and Telegram handler API calls. |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses Copilot review on PR #103: `z.coerce.number().int()` was too lax β it happily turns `""`/`null` into `0` and `true` into `1`, both of which then pass `.int()` and reach the bot API. Replace ad-hoc `z.coerce.number().int()` with a single shared `idSchema` in `src/core/tools/schemas.ts`: z.union([ z.number().int().positive(), z.string().regex(/^\d+$/).transform(Number).pipe(z.number().int().positive()), ]) Accepts: - 2081 β 2081 - "2081" β 2081 Rejects: - "" / " " (empty / whitespace strings) - "abc" / "1abc" (non-digit / mixed strings) - null / undefined - true / false - 0 / negative - 1.5 / non-integer Applied across messaging.ts, history.ts, members.ts, stickers.ts β 13 fields total. Audit test in `tool-id-coercion.test.ts` upgraded to walk every ID field on every tool and assert the full accept/reject contract (not just "accepts a numeric string"). 168 generated cases plus 56 hand-rolled spot-checks = 224 ID-coercion assertions, all green. Total: 1635 / 1635 tests passing, tsc clean, lint unchanged.
|
Addressed Copilot's review in commit 1. z.union([
z.number().int().positive(),
z.string().regex(/^\d+$/).transform(Number).pipe(z.number().int().positive()),
])Now rejects: 2. Audit test was too loose (called out on 3. PR description coverage (called out on Final verification:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Use this instead of `z.number()` or `z.coerce.number()` for any ID | ||
| * field on tool input schemas. The plain coercion path was too lax β | ||
| * `z.coerce.number().int()` happily turns `null`/`""` into `0` and | ||
| * `true` into `1`, both of which the Telegram bot API would then | ||
| * dispatch to. The strict union avoids that. | ||
| */ |
The postinstall hook in package.json runs scripts/prune-native-sdk.mjs automatically during npm ci. Without COPY scripts/ scripts/ before the npm ci step, Docker fails with 'npm error command failed: node scripts/ prune-native-sdk.mjs'. This was inadvertently dropped when resolving Dockerfile conflicts during the rebase onto main (hb #103). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The postinstall hook in package.json runs scripts/prune-native-sdk.mjs automatically during npm ci. Without COPY scripts/ scripts/ before the npm ci step, Docker fails with 'npm error command failed: node scripts/ prune-native-sdk.mjs'. This was inadvertently dropped when resolving Dockerfile conflicts during the rebase onto main (hb #103). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The postinstall hook in package.json runs scripts/prune-native-sdk.mjs automatically during npm ci. Without COPY scripts/ scripts/ before the npm ci step, Docker fails with 'npm error command failed: node scripts/ prune-native-sdk.mjs'. This was inadvertently dropped when resolving Dockerfile conflicts during the rebase onto main (hb #103). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The postinstall hook in package.json runs scripts/prune-native-sdk.mjs automatically during npm ci. Without COPY scripts/ scripts/ before the npm ci step, Docker fails with 'npm error command failed: node scripts/ prune-native-sdk.mjs'. This was inadvertently dropped when resolving Dockerfile conflicts during the rebase onto main (hb #103). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
β¦w, lockfile portability, tarball validation Per Dylan's "overhaul the CI and make it much more comprehensive and robust." Five new jobs gated through the existing `CI Status` aggregate, plus a real package-distribution fix the new tarball checker uncovered. ## What's new ### `integration` β MCP-functional tier in CI (Ubuntu/macOS/Windows Γ Node 22) The new MCP-functional tier from #131 (`talon-mcp-functional.test.ts`) now runs on every PR + push across the full OS matrix. Catches SDKβMCPβbridgeβhandler integration bugs (the PR #122 prefix-mismatch class) without needing a live external service. Builds the stub-claude SEA binary on Windows (Node 20.19+ blocks spawning .cmd shims via child_process.spawn β CVE-2024-27980 mitigation). ### `secrets` β gitleaks scan on every push + PR `gitleaks/gitleaks-action@v2` with `fetch-depth: 0` so it sees full git history, not just the latest commit. Catches API keys, bot tokens, .env files committed by accident, OAuth secrets, private keys. ### `dep-review` β github/dependency-review-action on PRs Diffs lockfile changes against the GitHub Advisory Database. `fail-on-severity: moderate` β moderate or higher CVE in any newly introduced dep blocks the PR. Comments summary on PR on failure. PR-only (no main-branch run); `ci-ok` treats `skipped` as ok. ### `lockfile` β portability check in fresh tmpdir Copies ONLY `package.json` + `package-lock.json` to a fresh tmpdir and runs `npm ci --ignore-scripts --no-audit` there. Same check as `tools/verify-lockfile.sh` locally. Catches the "lockfile drift" / "lockfile only validates with `--include=optional`" classes the heartbeat journal documented (hb #48 / #103). The other test jobs run `npm ci` in the source tree where `node_modules` state could mask portability bugs. This one starts from zero, every run. ### `tarball` β validates `npm pack` contents New `.github/scripts/tarball-check.mjs` β runs `npm pack`, inspects the resulting tarball for forbidden patterns: - `.env*`, `credentials.json`, `*.key`, `*.pem`, `id_rsa`, etc. - `__tests__/`, `*.test.ts`, `*.spec.ts` - `node_modules`, `.git`, `coverage`, sourcemaps, `.vscode`, `.idea`, `.DS_Store` - Tarball size > 5MB (catches accidental large-file commits) - `package.json` / `README` missing Also prints a sorted top-10 largest files for the CI step summary. ## Real bug uncovered The tarball checker's first run flagged **270+ test files were being shipped in the npm tarball**. `package.json#files` listed `"src/"` which swept in the entire test suite. Fixed by replacing the broad `"src/"` entry with explicit subdirs: - "src/backend/", "src/core/", "src/frontend/", "src/plugins/", "src/storage/", "src/util/" - "src/bootstrap.ts", "src/cli.ts", "src/index.ts", "src/login.ts" Tarball size dropped from including 270+ test files to a clean 99 source-only files (180KB β still 180KB but with no test scaffolding). Future versions of `talon-agent` on npm won't ship test fixtures. ## Coverage config `vitest.config.ts`: added an exclude list (test files, integration scaffolding, entry points) and `lcov` reporter for tooling compatibility (codecov, IDE plugins). Thresholds left at 60% for now β ratcheting up is out of scope for this PR (would risk breaking CI on unrelated paths). ## ci-ok gate Updated to require all 10 jobs (was 6). Branch protection in `.github/branch-protection.json` already gates merges on `CI Status`, so adding a new required job means adding it to the `needs:` list + the `check` calls in the gate script β no separate config change. ## Test plan - [x] `tsc --noEmit` clean - [x] `prettier --check` clean on touched files - [x] `npm run test:integration` passes locally (4 tests) - [x] `npm run tarball:check` passes locally β clean tarball - [x] Lockfile portability passes (replicated tmpdir check, 7s npm ci) - [x] YAML syntax valid - [x] Existing test suite unaffected (1711 tests pass + 1 pre-existing main flake)
β¦iew + lockfile portability + tarball validation (#137) * ci: comprehensive overhaul β integration tier, secret scan, dep review, lockfile portability, tarball validation Per Dylan's "overhaul the CI and make it much more comprehensive and robust." Five new jobs gated through the existing `CI Status` aggregate, plus a real package-distribution fix the new tarball checker uncovered. ## What's new ### `integration` β MCP-functional tier in CI (Ubuntu/macOS/Windows Γ Node 22) The new MCP-functional tier from #131 (`talon-mcp-functional.test.ts`) now runs on every PR + push across the full OS matrix. Catches SDKβMCPβbridgeβhandler integration bugs (the PR #122 prefix-mismatch class) without needing a live external service. Builds the stub-claude SEA binary on Windows (Node 20.19+ blocks spawning .cmd shims via child_process.spawn β CVE-2024-27980 mitigation). ### `secrets` β gitleaks scan on every push + PR `gitleaks/gitleaks-action@v2` with `fetch-depth: 0` so it sees full git history, not just the latest commit. Catches API keys, bot tokens, .env files committed by accident, OAuth secrets, private keys. ### `dep-review` β github/dependency-review-action on PRs Diffs lockfile changes against the GitHub Advisory Database. `fail-on-severity: moderate` β moderate or higher CVE in any newly introduced dep blocks the PR. Comments summary on PR on failure. PR-only (no main-branch run); `ci-ok` treats `skipped` as ok. ### `lockfile` β portability check in fresh tmpdir Copies ONLY `package.json` + `package-lock.json` to a fresh tmpdir and runs `npm ci --ignore-scripts --no-audit` there. Same check as `tools/verify-lockfile.sh` locally. Catches the "lockfile drift" / "lockfile only validates with `--include=optional`" classes the heartbeat journal documented (hb #48 / #103). The other test jobs run `npm ci` in the source tree where `node_modules` state could mask portability bugs. This one starts from zero, every run. ### `tarball` β validates `npm pack` contents New `.github/scripts/tarball-check.mjs` β runs `npm pack`, inspects the resulting tarball for forbidden patterns: - `.env*`, `credentials.json`, `*.key`, `*.pem`, `id_rsa`, etc. - `__tests__/`, `*.test.ts`, `*.spec.ts` - `node_modules`, `.git`, `coverage`, sourcemaps, `.vscode`, `.idea`, `.DS_Store` - Tarball size > 5MB (catches accidental large-file commits) - `package.json` / `README` missing Also prints a sorted top-10 largest files for the CI step summary. ## Real bug uncovered The tarball checker's first run flagged **270+ test files were being shipped in the npm tarball**. `package.json#files` listed `"src/"` which swept in the entire test suite. Fixed by replacing the broad `"src/"` entry with explicit subdirs: - "src/backend/", "src/core/", "src/frontend/", "src/plugins/", "src/storage/", "src/util/" - "src/bootstrap.ts", "src/cli.ts", "src/index.ts", "src/login.ts" Tarball size dropped from including 270+ test files to a clean 99 source-only files (180KB β still 180KB but with no test scaffolding). Future versions of `talon-agent` on npm won't ship test fixtures. ## Coverage config `vitest.config.ts`: added an exclude list (test files, integration scaffolding, entry points) and `lcov` reporter for tooling compatibility (codecov, IDE plugins). Thresholds left at 60% for now β ratcheting up is out of scope for this PR (would risk breaking CI on unrelated paths). ## ci-ok gate Updated to require all 10 jobs (was 6). Branch protection in `.github/branch-protection.json` already gates merges on `CI Status`, so adding a new required job means adding it to the `needs:` list + the `check` calls in the gate script β no separate config change. ## Test plan - [x] `tsc --noEmit` clean - [x] `prettier --check` clean on touched files - [x] `npm run test:integration` passes locally (4 tests) - [x] `npm run tarball:check` passes locally β clean tarball - [x] Lockfile portability passes (replicated tmpdir check, 7s npm ci) - [x] YAML syntax valid - [x] Existing test suite unaffected (1711 tests pass + 1 pre-existing main flake) * ci: drop Windows from integration job β MCP-dispatch chain doesn't run there yet The MCP-functional tier passes on Ubuntu + macOS but fails on Windows: the stub's SEA-bundled MCP client + StdioClientTransport spawn-and-fetch chain to the recording handler doesn't complete under SEA-on-Windows. Three of four cases see zero captures at the recording handler, even though `PostToolBatch: terminating SDK loop on mcp__telegram-tools__end_turn` fires (so the SDK loop sees the tool, but MCP dispatch never lands). Likely culprits to investigate (out of scope for this PR): - esbuild SEA bundle handling of the dynamic `await import("@modelcontextprotocol/sdk/...")` - StdioClientTransport.spawn behavior under SEA-on-Windows - Localhost loopback / fetch from spawned subprocess on Windows The other Windows test paths still run β `Tests (windows-latest, Node 22)` and `Functional (windows-latest)` cover the platform without MCP dispatch (sdk-stub, talon-functional). Reinstating Windows for `integration` is gated on root-causing the failure. * Revert "ci: drop Windows from integration job β MCP-dispatch chain doesn't run there yet" This reverts commit 6410446. * fix(integration): static MCP imports + protocol-log-on-failure for Windows debugging Two changes targeting the Windows integration failure: 1. fake-claude.mjs: convert dynamic `await import("@modelcontextprotocol/sdk/...")` to static top-level imports. Likely root cause of the Windows failure β esbuild's CJS bundling preserves `await import(literal)` as `Promise.resolve(require(literal))`, which works locally where node_modules exists, but the SEA binary on Windows ships only the bundled .cjs without node_modules. Static imports force esbuild to inline the SDK module bodies into the bundle. Verified: rebuilt SEA bundle locally, `grep "var StdioClientTransport" fake-claude.cjs` confirms the SDK is now inlined. 2. talon-mcp-functional.test.ts: add `withProtocolLogOnFailure()` helper. The MCP-functional tier has subprocess hops (SDK loop β MCP server spawn β bridge HTTP) that produce no Vitest output when something goes wrong upstream of the actual `expect(...)`. This wrapper dumps the last 60 lines of the stub's protocol log on assertion failure so future Windows regressions are diagnosable from the CI log directly. Wraps the canary first test only (the others' assertions are all downstream of the same MCP path). Local: all 11 MCP-functional tests still pass on Linux. Windows verification has to come from the CI run on this branch. * fix(mcp-server): unify Windows + POSIX command path β node --import tsx, not npx tsx Real root cause of the Windows integration failure. `buildMcpServers` was spawning `npx tsx <path>` on Windows and `node --import <tsx-loader> <path>` on POSIX. The Windows path goes through the MCP launcher (which proxies stdio), so the launcher then runs `child_process.spawn("npx", [...])` to start the actual MCP server. Node 20.19+ refuses to execute `.cmd` shims via `child_process.spawn` without `shell: true` (CVE-2024-27980 mitigation). `npx` on Windows IS a .cmd shim. Result: the launcher's spawn fails immediately, the MCP server never starts, the stub's MCP client tries to connect over a dead pipe and gets `MCP error -32000: Connection closed`. The earlier hypothesis (SEA bundle missing dynamic imports) was wrong β the static-imports change in 9507333 is still the right fix (removes a dynamic-import gotcha entirely), but the actual blocker was this command shape. Fix: use `node --import <tsx-loader> <path>` on every platform. tsx as a Node loader is platform-identical, and we never spawn `npx.cmd`. The launcher's spawn now hits a plain `node` binary, which works under Node's CVE mitigation. Verified locally: 11 / 11 MCP-functional tests still pass on Linux. Windows verification: this branch's CI run. Diagnostic from the previous failed Windows CI run that pinned this: MCP_CONFIG servers: ["telegram-tools"] MCP connect error for telegram-tools: MCP error -32000: Connection closed The launcher's spawned child died before completing initialize. * fix(mcp-server): convert tsx loader path to file:// URL for Windows Second Windows iteration. Same `MCP error -32000: Connection closed` after the previous fix, even with `node --import <path>` everywhere. The remaining issue: `--import` accepts URLs or paths, but on Windows a raw backslash path like `D:\a\talon\talon\node_modules\tsx\dist\esm\index.mjs` is ambiguous between path and URL. Node's loader-hook resolution silently fails to register tsx, every subsequent `.ts` import in `mcp-server.ts` throws, the MCP server crashes, the stub's MCP client gets "Connection closed" on its initialize handshake. Fix: convert via `pathToFileURL(resolve(...)).href` so the loader URL is always a clean `file:///D:/.../index.mjs` regardless of platform. Linux already worked because `/path/to/foo.mjs` is unambiguous as both a path and a URL. Local: 11 / 11 still pass on Linux. Windows: this commit's CI run is the truth-or-not test.
Summary
The
reacttool (and friends βpin_message,get_member_info,download_media, etc.) were occasionally failing with:β¦even when the model formatted the call correctly for a JSON Schema number field. The bug surfaced live in DM mid-conversation when
react(message_id=2081, emoji="β€οΈ")rejected β same model output worked forsend(reply_to=2081)because that field is.optional()and apparently took a different path.Cause
Some MCP transport / model paths deliver
message_id,user_id,reply_to,offset_idas JSON strings ("2081") rather than numbers (2081). Plainz.number()rejects those. The bridge handlers inactions.tsalready doNumber(body.message_id), so they're tolerant β but the schema validation runs first and rejects before the handler ever sees the call.Fix
Switch every ID-shaped field in tool input schemas to
z.coerce.number().int():"2081"β2081)"abc"βNaNβ fails.int())1.5β fails.int())Affected fields:
message_id,user_id,reply_to,offset_idacrossmessaging.ts,members.ts,stickers.ts,history.ts(13 occurrences total).Tests
New file:
src/__tests__/tool-id-coercion.test.tsALL_TOOLSschema and fails if any field whose name is in{message_id, user_id, reply_to, offset_id}still uses rawz.number(). This caught one I missed on the first pass (download_media) β kept in to prevent future regressions.Verification
tsc --noEmitcleannpm run lintunchanged (9 pre-existing warnings, 0 errors)Test plan
react(message_id=N, emoji="β€οΈ")β should succeedpin_message,get_member_info,download_mediadelete_message,forward_message,edit_message,stop_poll,unpin_messageβ sanity-check still work with regular number inputsπ€ Generated with Claude Code