Fix/paginated bigquery schema - #1
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds BigQuery schema discovery, MCP HTTP and telemetry infrastructure, structured Redash logging, pnpm-based builds, Docker packaging, and release automation. ChangesMCP server platform
BigQuery schema discovery
Toolchain and release automation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant BigQuerySchemaService
participant RedashClient
MCPClient->>BigQuerySchemaService: Request paginated BigQuery schema data
BigQuerySchemaService->>RedashClient: Resolve data source and execute INFORMATION_SCHEMA query
RedashClient-->>BigQuerySchemaService: Return Redash query rows
BigQuerySchemaService-->>MCPClient: Return mapped pagination results
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2d86607 to
04b051c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/__tests__/redashClient.test.ts (1)
441-457: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing error-path test for
getDataSource.Only the happy path is covered.
resolveDataSourceinbigQuerySchema.tsdepends ongetDataSourcethrowing (e.g. on a 403 for non-admin users) to trigger its permission fallback — an error-branch test here (mockmockAxiosInstance.getto reject and assert the wrappedFailed to fetch data source 4 from Redashmessage) would verify that contract end-to-end, mirroring the error tests likely already present for sibling methods in this suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/redashClient.test.ts` around lines 441 - 457, Add an error-path test alongside the existing getDataSource test, mock mockAxiosInstance.get to reject, and assert that client.getDataSource(4) rejects with the wrapped message “Failed to fetch data source 4 from Redash.”src/bigQuerySchema.ts (2)
163-191: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent fallback swallows real errors, not just permission gaps.
Both
catchblocks here are empty, so a genuine network/5xx failure fromgetDataSource(orgetDataSources) is indistinguishable from an expected permission-denied response — you only ever see the generic "Unable to determine data source ... type" message with no trace of the underlying cause. Given this fallback is safety-critical (it gates whether BigQuery schema discovery is allowed at all), logging the caught error before falling through would make production issues much easier to diagnose.🔍 Suggested logging addition
try { details = await this.client.getDataSource(dataSourceId); if (details.type) { return details; } - } catch { - // A non-admin Redash user might not be allowed to read full details. - // Fall back to the list endpoint, which still includes the data source type. + } catch (error) { + // A non-admin Redash user might not be allowed to read full details. + // Fall back to the list endpoint, which still includes the data source type. + logger.warn(`getDataSource(${dataSourceId}) failed, falling back to list: ${error}`); } try { const listed = (await this.client.getDataSources()).find((dataSource) => dataSource.id === dataSourceId); if (listed?.type) { return { ...listed, ...details, options: details.options ?? listed.options, }; } - } catch { - // Report one stable, safety-oriented error below. + } catch (error) { + // Report one stable, safety-oriented error below. + logger.warn(`getDataSources() fallback failed for ${dataSourceId}: ${error}`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bigQuerySchema.ts` around lines 163 - 191, Update both catch blocks in resolveDataSource to log the caught error before falling back, including which client request failed and the dataSourceId. Preserve the existing fallback behavior and final safety error, while ensuring unexpected failures from getDataSource or getDataSources remain diagnosable.
120-123: 🚀 Performance & Scalability | 🔵 TrivialOFFSET-based pagination will slow down at deep pages.
paginationSqlusesLIMIT/OFFSET; BigQuery must scan and discard all skipped rows for every request, so very deep pages (offset can reach ~1e8 givenMAX_PAGE/MAX_PAGE_SIZE) will get progressively slower and costlier. Not a bug given the bounded page sizes, but worth keeping in mind if callers page deeply through large datasets/tables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bigQuerySchema.ts` around lines 120 - 123, Keep paginationSql unchanged; the review identifies a scalability consideration rather than a required defect. Preserve the existing bounded page and page-size behavior, and defer cursor-based pagination until deep-page performance becomes an explicit requirement.src/__tests__/bigQuerySchema.test.ts (1)
63-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFallback-merge path is only exercised for
getSchema, not forlistDatasets/listTables.This test proves the type-detection fallback unblocks the safety check, but
resolveDataSource'soptions: details.options ?? listed.optionsmerge (which suppliesprojectId/locationforlistDatasets/listTables/getTableSchema) is never exercised through this fallback branch. A test wheregetDataSourcereturns{ view_only: true }(no options) andgetDataSourcesreturns the full entry withoptions, then assertinglistDatasetsstill resolvesprojectId/locationcorrectly, would close that gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/bigQuerySchema.test.ts` around lines 63 - 71, The fallback-merge behavior in resolveDataSource is untested for dataset listing. Add a test in bigQuerySchema.test.ts where getDataSource returns only view_only, getDataSources returns the matching BigQuery entry with options including projectId and location, and listDatasets is invoked; assert the operation resolves using those fallback options..github/workflows/release.yml (1)
35-38: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftMigrate both npm publishing paths to trusted publishing.
.github/workflows/release.yml#L35-L38: configure npm OIDC publishing, addid-token: write, and removeNPM_TOKEN..github/workflows/tagpr.yml#L50-L54: apply the same migration. npm recommends trusted publishing over long-lived tokens and generates provenance automatically. (docs.npmjs.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 35 - 38, Update the Publish to npm job in .github/workflows/release.yml at lines 35-38 to use npm trusted publishing: grant id-token: write permissions and remove the NPM_TOKEN environment variable. Apply the same change to the npm publishing path in .github/workflows/tagpr.yml at lines 50-54, preserving the existing publish behavior while relying on OIDC provenance.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Line 13: Disable persisted checkout credentials in the checkout steps of
.github/workflows/test.yml (lines 13-13), .github/workflows/tagpr.yml (lines
17-19), and .github/workflows/release.yml (lines 12-12) by setting
persist-credentials to false; no other workflow behavior should change.
In @.npmrc:
- Line 2: Remove the committed ignore-scripts=true setting from .npmrc so npm
publish runs the package lifecycle scripts, including prepublishOnly, while
leaving the CI build behavior unchanged.
In `@package.json`:
- Line 6: Align the Node.js runtime with the pnpm 11.6.0 requirement: update the
Node version in package.json, .github/workflows/test.yml,
.github/workflows/tagpr.yml, and .github/workflows/release.yml to Node 22 or
newer, keeping the packageManager pin consistent across all CI and release
workflows.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 35-38: Update the Publish to npm job in
.github/workflows/release.yml at lines 35-38 to use npm trusted publishing:
grant id-token: write permissions and remove the NPM_TOKEN environment variable.
Apply the same change to the npm publishing path in .github/workflows/tagpr.yml
at lines 50-54, preserving the existing publish behavior while relying on OIDC
provenance.
In `@src/__tests__/bigQuerySchema.test.ts`:
- Around line 63-71: The fallback-merge behavior in resolveDataSource is
untested for dataset listing. Add a test in bigQuerySchema.test.ts where
getDataSource returns only view_only, getDataSources returns the matching
BigQuery entry with options including projectId and location, and listDatasets
is invoked; assert the operation resolves using those fallback options.
In `@src/__tests__/redashClient.test.ts`:
- Around line 441-457: Add an error-path test alongside the existing
getDataSource test, mock mockAxiosInstance.get to reject, and assert that
client.getDataSource(4) rejects with the wrapped message “Failed to fetch data
source 4 from Redash.”
In `@src/bigQuerySchema.ts`:
- Around line 163-191: Update both catch blocks in resolveDataSource to log the
caught error before falling back, including which client request failed and the
dataSourceId. Preserve the existing fallback behavior and final safety error,
while ensuring unexpected failures from getDataSource or getDataSources remain
diagnosable.
- Around line 120-123: Keep paginationSql unchanged; the review identifies a
scalability consideration rather than a required defect. Preserve the existing
bounded page and page-size behavior, and defer cursor-based pagination until
deep-page performance becomes an explicit requirement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7adf3902-a21d-482e-a5f8-4574d737be0a
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.github/workflows/release.yml.github/workflows/tagpr.yml.github/workflows/test.yml.npmrcREADME.mdpackage.jsonsrc/__tests__/bigQuerySchema.test.tssrc/__tests__/redashClient.test.tssrc/bigQuerySchema.tssrc/index.tssrc/redashClient.ts
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' .github/workflows/test.yml
printf '\n---\n'
sed -n '1,260p' .github/workflows/tagpr.yml
printf '\n---\n'
sed -n '1,220p' .github/workflows/release.ymlRepository: kahirokunn/redash-mcp
Length of output: 2734
🏁 Script executed:
rg -n "git (push|commit|tag)|checkout@v6|persist-credentials|tagpr" .github/workflows README* . -g '!**/node_modules/**'Repository: kahirokunn/redash-mcp
Length of output: 1227
🌐 Web query:
actions/checkout persist-credentials default tagpr GITHUB_TOKEN git push workflow
💡 Result:
The actions/checkout action has a default behavior for the persist-credentials input of true [1][2][3]. When set to true (the default), the action automatically configures the local Git repository's configuration with the authentication token or SSH key used to clone the repository [1][3][4]. This allows subsequent steps in the same job to perform authenticated Git operations, such as git push, without needing to manually provide credentials [1][3]. When using tools like tagpr, which need to create pull requests, commits, or tags via Git, it is common to set persist-credentials: false [5][6]. This is done to prevent the automatic configuration of the GITHUB_TOKEN or other credentials in the local Git config [3][4]. When persist-credentials is set to false, you must explicitly manage authentication for Git commands, often by using the GITHUB_TOKEN provided via an environment variable (as tagpr does) [5][6]. To successfully use GITHUB_TOKEN for git push operations in a workflow: 1. Permission Configuration: You must grant the GITHUB_TOKEN the necessary write permissions [7][8]. This is done using the permissions key at the workflow or job level [9][7]. For tag creation and pushing, you typically need at least: permissions: contents: write pull-requests: write 2. Workflow Implementation: When using tagpr, the standard pattern is: steps: - uses: actions/checkout@v6 with: persist-credentials: false - uses: Songmu/tagpr@v1 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} By setting persist-credentials: false, you ensure that the checkout action does not persist the default token into the local git config, and instead, the tagpr action uses the GITHUB_TOKEN provided in its environment to perform the required Git operations [5][6].
Citations:
- 1: https://github.com/actions/checkout/blob/master/adrs/0153-checkout-v2.md
- 2: https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/action.yml
- 3: https://github.com/actions/checkout
- 4: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 5: https://github.com/marketplace/actions/automate-pull-request-generation-and-tagging-for-releases-using-tagpr
- 6: https://github.com/Songmu/tagpr
- 7: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
- 8: https://stackoverflow.com/questions/72383123/what-permissions-are-needed-for-github-actions-to-create-a-tag-and-release-for-a
- 9: https://docs.github.com/actions/reference/authentication-in-a-workflow
Disable persisted checkout credentials in these workflows.
.github/workflows/test.ymland.github/workflows/release.yml: addpersist-credentials: false; neither job needs git auth after checkout..github/workflows/tagpr.yml: addpersist-credentials: false;Songmu/tagpralready getsGITHUB_TOKENfrom the environment.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 13-13: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 3 files
.github/workflows/test.yml#L13-L13(this comment).github/workflows/tagpr.yml#L17-L19.github/workflows/release.yml#L12-L12
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml at line 13, Disable persisted checkout
credentials in the checkout steps of .github/workflows/test.yml (lines 13-13),
.github/workflows/tagpr.yml (lines 17-19), and .github/workflows/release.yml
(lines 12-12) by setting persist-credentials to false; no other workflow
behavior should change.
Source: Linters/SAST tools
| @@ -0,0 +1,2 @@ | |||
| package-lock=false | |||
| ignore-scripts=true | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files .npmrc package.json .github/workflows 2>/dev/null || true
printf '\n## .npmrc\n'
if [ -f .npmrc ]; then cat -n .npmrc; fi
printf '\n## package.json (relevant scripts)\n'
if [ -f package.json ]; then
python3 - <<'PY'
import json
from pathlib import Path
p = Path('package.json')
data = json.loads(p.read_text())
scripts = data.get('scripts', {})
for k in sorted(scripts):
if any(x in k.lower() for x in ['publish','build','prepublish','prepack','prepare','postinstall','install']):
print(f"{k}: {scripts[k]}")
PY
fi
printf '\n## workflow references\n'
rg -n "npm publish|prepublishOnly|ignore-scripts|npm pack|pnpm publish|yarn publish|changeset|dist/" .github/workflows package.json .npmrc -S || trueRepository: kahirokunn/redash-mcp
Length of output: 1416
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Repository file map (top level)\n'
fd -d 2 -t f 'package.json|.npmrc|.*workflow.*' . 2>/dev/null | sed 's#^\./##' | sort || trueRepository: kahirokunn/redash-mcp
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## .npmrc'
cat -n .npmrc 2>/dev/null || true
echo '## package.json scripts'
python3 - <<'PY'
import json
from pathlib import Path
p = Path('package.json')
if p.exists():
data = json.loads(p.read_text())
for k, v in sorted(data.get('scripts', {}).items()):
if any(x in k.lower() for x in ['build', 'publish', 'prepublish', 'prepack', 'prepare', 'install']):
print(f'{k}: {v}')
PY
echo '## workflow references'
rg -n "ignore-scripts|prepublishOnly|npm publish|npm pack|pnpm publish|yarn publish|dist/" .github .npmrc package.json -S || trueRepository: kahirokunn/redash-mcp
Length of output: 1287
🌐 Web query:
npm ignore-scripts prepublishOnly publish lifecycle scripts documentation
💡 Result:
In npm, lifecycle scripts define the sequence of events that occur during commands like npm publish [1]. The prepublishOnly script is designed to run exclusively during npm publish, occurring before the package is prepared and packed [1][2]. Regarding the behavior of lifecycle scripts and the --ignore-scripts flag: 1. The --ignore-scripts configuration flag is intended to prevent the execution of scripts defined in package.json [3][4]. 2. Historically, there have been inconsistencies and bugs regarding whether --ignore-scripts correctly suppresses certain lifecycle hooks during operations like npm publish or npm pack [5][6]. 3. Specifically, for commands like npm publish, behavior regarding --ignore-scripts has been updated in newer versions of npm (such as the fix introduced in npm v7.20.0) to better respect this flag [5]. 4. It is important to note that for commands explicitly intended to run a specific script (like npm start, npm test, or npm run <script>), the requested script will still execute even if --ignore-scripts is set, though its associated pre- and post- scripts will be skipped [3][4]. 5. The prepublish script itself is deprecated [1][7]. It was originally intended to run on both npm publish and npm install, which caused confusion. As a result, the prepare event (which runs on npm install and npm publish) and prepublishOnly (which runs only on npm publish) were introduced as replacements [1][2]. For authoritative details on the current behavior of lifecycle scripts in your specific version of npm, refer to the documentation for your installed version at docs.npmjs.com [1][4].
Citations:
- 1: https://docs.npmjs.com/cli/v11/using-npm/scripts/
- 2: https://docs.npmjs.com/cli/v8/using-npm/scripts/?v=true
- 3: https://docs.npmjs.com/cli/v9/using-npm/config/
- 4: https://docs.npmjs.com/cli/v11/using-npm/config/?v=true
- 5: [BUG] Neither --ignore-scripts nor --ignore-prepublish are respected by npm publish npm/cli#2755
- 6: [BUG] npm pack and publish should not run hooks, or hooks need to have opt-out npm/cli#7211
- 7: [DOCS] npm publish - lifecycle event order discrepancy npm/cli#4893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## dist files tracked by git\n'
git ls-files dist 2>/dev/null || true
printf '\n## release workflow\n'
cat -n .github/workflows/release.yml
printf '\n## tagpr workflow\n'
cat -n .github/workflows/tagpr.yml
printf '\n## package.json relevant file refs\n'
python3 - <<'PY'
import json
from pathlib import Path
data = json.loads(Path('package.json').read_text())
for key in ['main','bin','files']:
if key in data:
print(f'{key}: {data[key]}')
PYRepository: kahirokunn/redash-mcp
Length of output: 2944
Scope ignore-scripts out of .npmrc. CI already builds before publish, but this committed default still disables prepublishOnly for manual npm publish, which can ship stale or missing dist/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.npmrc at line 2, Remove the committed ignore-scripts=true setting from
.npmrc so npm publish runs the package lifecycle scripts, including
prepublishOnly, while leaving the CI build behavior unchanged.
| "version": "0.0.13", | ||
| "description": "MCP server for Redash integration", | ||
| "type": "module", | ||
| "packageManager": "pnpm@11.6.0", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== package.json ==\n'
cat -n package.json | sed -n '1,40p'
printf '\n== .github/workflows/test.yml ==\n'
cat -n .github/workflows/test.yml | sed -n '1,80p'
printf '\n== .github/workflows/tagpr.yml ==\n'
cat -n .github/workflows/tagpr.yml | sed -n '1,120p'
printf '\n== .github/workflows/release.yml ==\n'
cat -n .github/workflows/release.yml | sed -n '1,120p'Repository: kahirokunn/redash-mcp
Length of output: 5064
🌐 Web query:
pnpm 11.6.0 minimum Node.js version requirements official documentation
💡 Result:
The official minimum Node.js version requirement for pnpm 11.6.0 is Node.js v22 or newer [1][2]. Starting with pnpm v11, the software is distributed as pure ESM (ECMAScript modules), which necessitates this higher Node.js version [1][3]. If you are installing pnpm via methods other than the standalone executable or @pnpm/exe, you must have Node.js v22+ installed on your system [1][2]. The standalone executable (@pnpm/exe) comes packaged with its own Node.js runtime and can be used on systems where Node.js is not otherwise installed [1][2].
Citations:
- 1: https://pnpm.io/installation
- 2: https://pnpm.io/next/installation
- 3: https://pnpm.io/blog/releases/11.0
Align pnpm 11.6.0 with Node 22+
pnpm@11.6.0 requires Node 22+, but these workflows still run on Node 18/20:
package.json#L6-L6: keep this pin only if all CI/release runners move to Node 22+..github/workflows/test.yml#L24-L24: bump to Node 22+..github/workflows/tagpr.yml#L29-L29: bump to Node 22+..github/workflows/release.yml#L22-L22: bump to Node 22+.
📍 Affects 4 files
package.json#L6-L6(this comment).github/workflows/test.yml#L24-L24.github/workflows/tagpr.yml#L29-L29.github/workflows/release.yml#L22-L22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 6, Align the Node.js runtime with the pnpm 11.6.0
requirement: update the Node version in package.json,
.github/workflows/test.yml, .github/workflows/tagpr.yml, and
.github/workflows/release.yml to Node 22 or newer, keeping the packageManager
pin consistent across all CI and release workflows.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bigQuerySchema.ts (1)
9-11: 🚀 Performance & Scalability | 🔵 TrivialDeep-offset pagination on
INFORMATION_SCHEMAis not cached and always bills a minimum.Per BigQuery docs, INFORMATION_SCHEMA query results aren't cached and each query incurs at least a 10 MB minimum charge on-demand pricing. With
MAX_PAGE = 1_000_000andpageSizeup to 100, a client could request an OFFSET up to ~100,000,000, forcing BigQuery to scan and discard a large number of rows on every paginated call. Worth considering a smaller, more realisticMAX_PAGEbound and/or documenting the cost implication of paginating deep into large schemas.Also applies to: 123-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bigQuerySchema.ts` around lines 9 - 11, Reduce the MAX_PAGE bound in the pagination configuration to a realistic limit that prevents extremely deep INFORMATION_SCHEMA offsets, and document the minimum uncached query cost for pagination if documentation is maintained alongside these constants. Preserve DEFAULT_PAGE_SIZE and MAX_PAGE_SIZE behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/bigQuerySchema.ts`:
- Around line 9-11: Reduce the MAX_PAGE bound in the pagination configuration to
a realistic limit that prevents extremely deep INFORMATION_SCHEMA offsets, and
document the minimum uncached query cost for pagination if documentation is
maintained alongside these constants. Preserve DEFAULT_PAGE_SIZE and
MAX_PAGE_SIZE behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8beb7cc9-3cb0-40e7-a3ad-c4e6deb031dd
📒 Files selected for processing (6)
README.mdsrc/__tests__/bigQuerySchema.test.tssrc/__tests__/redashClient.test.tssrc/bigQuerySchema.tssrc/index.tssrc/redashClient.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/tests/redashClient.test.ts
- src/tests/bigQuerySchema.test.ts
- src/redashClient.ts
- README.md
- src/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
15-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the inspector CLI reproducible.
pnpm dlxfetches the inspector outside project dependencies and the lockfile. Since Playwright startsnpm run inspector, an upstream inspector change can break CI without a repository change. Add a tested inspector version todevDependenciesand invoke its local binary, or pin an exact version inpnpm dlx. (pnpm.io)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 15 - 24, Make the inspector commands reproducible by pinning the inspector CLI to an exact tested version. Update both inspector and inspector:headless scripts to use that pinned version, or add the package to devDependencies and invoke its local binary; ensure Playwright’s npm run inspector path uses the pinned dependency.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@package.json`:
- Around line 15-24: Make the inspector commands reproducible by pinning the
inspector CLI to an exact tested version. Update both inspector and
inspector:headless scripts to use that pinned version, or add the package to
devDependencies and invoke its local binary; ensure Playwright’s npm run
inspector path uses the pinned dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d8a3506-7daf-422b-a4c2-8ce720389333
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
package.json
Replace z.record(z.any()) for the schedule parameter in create_query and update_query with a proper Zod schema (scheduleSchema). The Zod schema sets day_of_week to null by default so callers no longer need to include it explicitly. This prevents malformed schedule objects that are missing the day_of_week key, which can break Redash's refresh_queries scheduler (ref: getredash/redash#4163). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move scheduleSchema to a dedicated module so tests can import it without triggering RedashClient initialization, which requires REDASH_URL and REDASH_API_KEY environment variables. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
44a2d2c to
afd20f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
101-101: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse one maintained Node.js baseline across the package, Docker image, workflows, and documentation.
Node.js 20 reached EOL on April 30, 2026, while Node.js 22 remains supported. The repository still targets Node.js 20 in the package engine, Docker image stages, README prerequisites, and every CI job. Use Node.js 22 or another supported LTS release across all these files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 101, Update the Node.js baseline from 20 to a single supported LTS release, preferably 22, across package.json lines 101-101, Dockerfile lines 5-25, and README.md lines 25-26; also update every CI workflow’s Node.js version to match, preserving consistent engine, image, prerequisite, and CI configuration.
🧹 Nitpick comments (11)
src/redashClient.ts (1)
636-652: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider carrying the Redash job error into
QueryExecutionFailedError.
QueryExecutionFailedErrorproduces the fixed message "Query execution failed".executeQueryre-wraps it, so the MCP tool caller receives no cause. The Redash job error reaches the log sink only. For BigQuery schema discovery, the job error text carries the actionable reason, for example a permission denial or an unknown dataset.If you want to keep response bodies out of thrown errors, attach the job error as a non-enumerable
causeand keep the tool-facing message generic.♻️ Proposed refactor
class QueryExecutionFailedError extends Error { - constructor() { - super("Query execution failed"); + constructor(options?: { cause?: unknown }) { + super("Query execution failed", options); this.name = "QueryExecutionFailedError"; } }- throw new QueryExecutionFailedError(); + throw new QueryExecutionFailedError({ cause: response.data.job.error });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/redashClient.ts` around lines 636 - 652, Update the status-4 handling in executeQuery to preserve response.data.job.error when constructing QueryExecutionFailedError, attaching it as a non-enumerable cause while retaining the generic tool-facing message. Ensure the existing catch branch still rethrows this error unchanged and executeQuery does not discard the attached cause when re-wrapping it.src/packageInfo.ts (1)
13-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the version-resolution fallback chain.
readPackageVersionimplements several distinct code paths: the self-referencingnode_moduleslookup, the../package.jsonrelative lookup, thecwdRequirelookup, themanifest.nameguard, and the final"unknown"fallback. No test file for this module is present in the reviewed set.PACKAGE_VERSIONfrom this module is used directly in server metadata:version: PACKAGE_VERSION, and separately as theATTR_SERVICE_VERSIONfallback in telemetry resource creation. Because two independent consumers rely on this value, regressions in the fallback chain would silently degrade both the MCP server identity and telemetry service version without a test failure to catch it.Add tests that mock
node:module'screateRequire(or stubprocess.argv/process.cwd) to exercise each candidate succeeding, each candidate throwing, amanifest.namemismatch, an emptyversionstring, and the final"unknown"fallback.src/__tests__/httpServer.test.ts (1)
237-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the grace period constant instead of hardcoding
5_000.
FORCE_CLOSE_GRACE_PERIOD_MSinsrc/httpServer.tsis private. This test duplicates its value. If the production value increases,advanceTimersByTimeAsync(5_000)no longer reaches the force-close path and the test hangs until the Jest timeout. Export the constant and import it here.♻️ Proposed change
In
src/httpServer.ts:-const FORCE_CLOSE_GRACE_PERIOD_MS = 5_000; +export const FORCE_CLOSE_GRACE_PERIOD_MS = 5_000;In this test:
-import { startHttpServer, type HttpServerHandle } from "../httpServer.js"; +import { + FORCE_CLOSE_GRACE_PERIOD_MS, + startHttpServer, + type HttpServerHandle, +} from "../httpServer.js";- await jest.advanceTimersByTimeAsync(5_000); + await jest.advanceTimersByTimeAsync(FORCE_CLOSE_GRACE_PERIOD_MS);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/httpServer.test.ts` around lines 237 - 245, Export the existing FORCE_CLOSE_GRACE_PERIOD_MS constant from the HTTP server module and import it in the test; replace the hardcoded 5_000 argument to jest.advanceTimersByTimeAsync in the socket shutdown test with that shared constant.src/__tests__/logger.test.ts (1)
40-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the trace correlation suffix.
Logger.logappendstrace_id=<id> span_id=<id>to the stderr line when a span context is active. Every assertion in this file runs without an active span, so that branch is untested. A regression in the correlation format would not fail any test. Add one test that runslogger.infoinside an active span and asserts the suffix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/logger.test.ts` around lines 40 - 54, Add a focused test alongside the existing logger level cases that starts an active span, calls Logger.info, and asserts consoleError receives the message with the exact trace_id and span_id suffix. Use the existing logger and tracing/test utilities, and ensure the span context is active during the call and cleaned up afterward.src/index.ts (2)
2358-2370: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftType the tool handler result instead of casting.
defineTooltypeshandleras(args: Record<string, unknown>) => Promise<unknown>, so line 2367 needsas CallToolResult. The cast removes compile-time checking for all 70 handlers. A handler that returns a malformed content shape fails only at runtime.Type the handler return value as
CallToolResultindefineToolso the compiler checks each handler.♻️ Proposed refactor
function defineTool<T extends ZodRawShape>( name: string, description: string, - handler: (args: z.output<ZodObject<T>>) => Promise<unknown>, + handler: (args: z.output<ZodObject<T>>) => Promise<CallToolResult>, schema: ZodObject<T> = emptyInputSchema as ZodObject<T>, ) { return { name, description, inputSchema: cacheJsonSchemaConversion(schema), - handler: handler as (args: Record<string, unknown>) => Promise<unknown>, + handler: handler as (args: Record<string, unknown>) => Promise<CallToolResult>, }; }Note: this change can surface existing type errors in the handlers, because
type: "text"literals widen tostring. Addas constor explicit return types where needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 2358 - 2370, Update defineTool so its handler return type is Promise<CallToolResult> rather than Promise<unknown>, then remove the `as CallToolResult` cast from the registered tool callback. Resolve any resulting handler type errors by preserving literal types with `as const` or adding explicit CallToolResult return types.
70-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid re-wrapping the shared
emptyInputSchemaconverter.
defineToolcallscacheJsonSchemaConversion(schema)for every tool. Tools without a schema all share the sameemptyInputSchemaobject. Each call reads the currentstd.jsonSchema(already a wrapper) and installs a new wrapper around it with a new cache map. The result is one nested closure layer per no-argument tool. Behavior stays correct, but the indirection grows with the tool count.Consider caching at the module level once per schema object.
♻️ Proposed refactor
+const cachedSchemas = new WeakSet<object>(); + function cacheJsonSchemaConversion<T extends ZodRawShape>(schema: ZodObject<T>): ZodObject<T> { + if (cachedSchemas.has(schema)) { + return schema; + } const std = schema["~standard"] as { jsonSchema?: JsonSchemaConverter }; const converter = std.jsonSchema; if (!converter) { return schema; } + cachedSchemas.add(schema);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 70 - 85, Update defineTool and the schema-conversion caching flow so the shared emptyInputSchema is converted once per schema object at module scope, rather than re-wrapping it for every schema-less tool. Reuse the cached conversion for repeated references to the same schema while preserving conversion behavior for distinct schemas.src/__tests__/mcpServer.test.ts (1)
97-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative-path test for resource URI validation.
The resource tests cover only valid
redash://query/11andredash://dashboard/22URIs.readRedashResourceinsrc/index.tsalso rejects unsupported types and invalid IDs. No test covers those branches, so the ID guard can regress without a failure. This is related to the lenientNumber.parseIntbehavior flagged onsrc/index.tslines 2163-2176.Add cases for an unsupported type and a non-numeric ID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/mcpServer.test.ts` around lines 97 - 147, Extend the resource tests around readResource to cover readRedashResource validation failures: assert that an unsupported redash URI type and a URI with a non-numeric ID are rejected with the expected errors. Reuse the existing connectDirectClient setup and ensure each case closes the connection.src/logger.ts (1)
71-85: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a minimum level threshold before writing to stderr and to the MCP client.
logwrites every record toconsole.errorand forwards every record tosendLoggingMessage, with no level check.Two consequences:
src/index.tsline 2366 emits a debug record for every tool request, andsrc/redashClient.tsemits debug records per HTTP call. All of them reach stderr in production.- The MCP protocol lets a client set its minimum level with
logging/setLevel. This code ignores that setting and sends a notification for every record, including debug, over the same stdio pipe that carries MCP messages.Store a minimum level and compare severity numbers before both sinks. Update the stored level when the client calls
logging/setLevel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logger.ts` around lines 71 - 85, Add a stored minimum log level to the logger and gate both console.error and server.sendLoggingMessage in log by comparing each record’s severity before writing or forwarding it. Handle logging/setLevel requests by updating that stored threshold, preserving existing error reporting for failed notifications.src/startup.ts (1)
49-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a bounded timeout to the shutdown path.
shutdownawaitshandle.close()with no time limit. For the HTTP transport,close()waits for open connections to drain. If one connection stays open, the process never exits after SIGTERM, and the supervisor sends SIGKILL after the grace period. Telemetry flush at line 65 is then also skipped.Race
handle.close()against a timeout, then continue to telemetry shutdown.♻️ Proposed change
+const SHUTDOWN_TIMEOUT_MS = 10_000; + try { - await handle.close(); + let timer: NodeJS.Timeout | undefined; + await Promise.race([ + handle.close(), + new Promise<never>((_, rejectTimeout) => { + timer = setTimeout( + () => rejectTimeout(new Error(`Shutdown timed out after ${SHUTDOWN_TIMEOUT_MS}ms`)), + SHUTDOWN_TIMEOUT_MS, + ).unref(); + }), + ]).finally(() => clearTimeout(timer)); target.exitCode ??= 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/startup.ts` around lines 49 - 70, Update the shutdown flow in shutdown to race handle.close() against a bounded timeout, ensuring the timeout rejects or otherwise completes the close attempt and allows execution to continue. Preserve the existing success and error logging, exit-code handling, and always invoke shutdownTelemetry() in finally after the timeout or close result.src/utils.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the duplicate unknown error formatter.
src/logger.ts:89definesformatUnknownwith the same behavior assrc/utils.ts:1’sformatError. ReuseformatErrorfromsrc/logger.tsif that does not create an import cycle; keepsrc/utils.tsindependent ofsrc/logger.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils.ts` around lines 1 - 3, Consolidate the duplicate formatting logic by updating logger.ts’s formatUnknown to reuse formatError from utils.ts, provided the import does not create a cycle. Keep formatError as the shared implementation in utils.ts, which must remain independent of logger.ts..github/workflows/release.yml (1)
176-176: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePrefer
env:over direct expression interpolation in the cosign step.
run: cosign sign --yes ghcr.io/${{ github.repository }}@${{ steps.build-and-push.outputs.digest }}interpolates GitHub Actions expressions directly into the shell command. Neither value is attacker-controlled here, but routing them throughenv:avoids the general template-injection pattern flagged by static analysis and keeps the workflow consistent with GitHub's hardening guidance.🔒 Proposed fix
- name: Sign Docker image - run: cosign sign --yes ghcr.io/${{ github.repository }}@${{ steps.build-and-push.outputs.digest }} + env: + IMAGE_REF: ghcr.io/${{ github.repository }}@${{ steps.build-and-push.outputs.digest }} + run: cosign sign --yes "${IMAGE_REF}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml at line 176, Update the cosign signing step to pass the repository and image digest through an env block, then reference those environment variables in the run command instead of directly interpolating github.repository and steps.build-and-push.outputs.digest. Preserve the existing signing target and behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Line 32: Update the actions/setup-node@v6 configuration in the pull-request
test workflow to set cache-write based on whether github.event_name is not
pull_request, and apply the same read-only condition to any other cache-writing
steps in that workflow.
- Around line 32-36: Update the release workflow’s Node setup and publish
configuration: for OIDC Trusted Publishing, use Node.js 22.14.0 or newer,
install npm 11.5.1 or newer before publishing, and remove registry-url or clean
the generated _authToken configuration so OIDC is selected; otherwise configure
the publish step to use NODE_AUTH_TOKEN from secrets.NPM_TOKEN for manual token
publishing.
In `@package.json`:
- Line 37: Update the package.json build script to replace the POSIX chmod
command with a Node-based executable-step, and update inspector:headless to set
BROWSER through the project’s cross-platform environment wrapper or shell
emulator instead of inline POSIX syntax. Preserve the existing command order and
behavior while making both scripts work under Windows’ default shell.
In `@src/__tests__/integration.test.ts`:
- Around line 8-24: The static axios import bypasses Jest’s mock in the ESM test
runtime. Replace jest.mock in the integration test with
jest.unstable_mockModule('axios', ...) and dynamically import axios and the
tested modules only after registering the mock, updating references such as
getRedashClient, RedashClient, toolDefinitions, and logger accordingly.
In `@src/index.ts`:
- Around line 2163-2176: Update readRedashResource to validate the extracted
variables.id as an entirely numeric value before parsing, rejecting values such
as “11abc”; only parse and continue with the existing safe-integer and
non-negative checks after strict validation.
- Around line 44-68: Add a focused runtime test for cacheJsonSchemaConversion
that uses a Zod schema exposing ~standard.jsonSchema, verifies the converter is
present, and confirms repeated input conversions for the same target are cached
rather than recomputed. Keep the test scoped to the existing conversion contract
and ensure it would fail if the converter moved to the MCP SDK fallback path.
In `@src/logger.ts`:
- Around line 122-144: Remove the redundant seen.add(fields) from
normalizeAttributes so nested objects are tracked only by
normalizeAttributeValue and removed correctly after processing. Preserve cycle
detection by ensuring the root fields object is added to the seen set at the log
entry point before normalization, so self-references remain reported as circular
while shared references normalize normally.
In `@src/redashClient.ts`:
- Around line 440-445: Update the redash.request.header.names field in
createQuery at src/redashClient.ts:440-445 and updateQuery at
src/redashClient.ts:492-500 to read this.client.defaults.headers?.common before
calling Object.keys, preserving the existing empty fallback so both methods log
actual header names instead of Axios method buckets.
In `@src/telemetry.ts`:
- Around line 398-403: Update parseDiagLogLevel to reject numeric normalized
values before checking DiagLogLevel membership, ensuring only non-numeric enum
keys are looked up and numeric OTEL_LOG_LEVEL inputs fall back to
DiagLogLevel.WARN.
---
Outside diff comments:
In `@package.json`:
- Line 101: Update the Node.js baseline from 20 to a single supported LTS
release, preferably 22, across package.json lines 101-101, Dockerfile lines
5-25, and README.md lines 25-26; also update every CI workflow’s Node.js version
to match, preserving consistent engine, image, prerequisite, and CI
configuration.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Line 176: Update the cosign signing step to pass the repository and image
digest through an env block, then reference those environment variables in the
run command instead of directly interpolating github.repository and
steps.build-and-push.outputs.digest. Preserve the existing signing target and
behavior.
In `@src/__tests__/httpServer.test.ts`:
- Around line 237-245: Export the existing FORCE_CLOSE_GRACE_PERIOD_MS constant
from the HTTP server module and import it in the test; replace the hardcoded
5_000 argument to jest.advanceTimersByTimeAsync in the socket shutdown test with
that shared constant.
In `@src/__tests__/logger.test.ts`:
- Around line 40-54: Add a focused test alongside the existing logger level
cases that starts an active span, calls Logger.info, and asserts consoleError
receives the message with the exact trace_id and span_id suffix. Use the
existing logger and tracing/test utilities, and ensure the span context is
active during the call and cleaned up afterward.
In `@src/__tests__/mcpServer.test.ts`:
- Around line 97-147: Extend the resource tests around readResource to cover
readRedashResource validation failures: assert that an unsupported redash URI
type and a URI with a non-numeric ID are rejected with the expected errors.
Reuse the existing connectDirectClient setup and ensure each case closes the
connection.
In `@src/index.ts`:
- Around line 2358-2370: Update defineTool so its handler return type is
Promise<CallToolResult> rather than Promise<unknown>, then remove the `as
CallToolResult` cast from the registered tool callback. Resolve any resulting
handler type errors by preserving literal types with `as const` or adding
explicit CallToolResult return types.
- Around line 70-85: Update defineTool and the schema-conversion caching flow so
the shared emptyInputSchema is converted once per schema object at module scope,
rather than re-wrapping it for every schema-less tool. Reuse the cached
conversion for repeated references to the same schema while preserving
conversion behavior for distinct schemas.
In `@src/logger.ts`:
- Around line 71-85: Add a stored minimum log level to the logger and gate both
console.error and server.sendLoggingMessage in log by comparing each record’s
severity before writing or forwarding it. Handle logging/setLevel requests by
updating that stored threshold, preserving existing error reporting for failed
notifications.
In `@src/redashClient.ts`:
- Around line 636-652: Update the status-4 handling in executeQuery to preserve
response.data.job.error when constructing QueryExecutionFailedError, attaching
it as a non-enumerable cause while retaining the generic tool-facing message.
Ensure the existing catch branch still rethrows this error unchanged and
executeQuery does not discard the attached cause when re-wrapping it.
In `@src/startup.ts`:
- Around line 49-70: Update the shutdown flow in shutdown to race handle.close()
against a bounded timeout, ensuring the timeout rejects or otherwise completes
the close attempt and allows execution to continue. Preserve the existing
success and error logging, exit-code handling, and always invoke
shutdownTelemetry() in finally after the timeout or close result.
In `@src/utils.ts`:
- Around line 1-3: Consolidate the duplicate formatting logic by updating
logger.ts’s formatUnknown to reuse formatError from utils.ts, provided the
import does not create a cycle. Keep formatError as the shared implementation in
utils.ts, which must remain independent of logger.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac08a963-08fe-46ba-b85c-03f19a11c56b
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
.dockerignore.env.example.github/workflows/release.yml.github/workflows/tagpr.yml.github/workflows/test.yml.gitignore.npmrcDockerfileREADME.mde2e/inspector-basic.spec.tsjest.config.jspackage.jsonplaywright.config.tssrc/__tests__/bigQuerySchema.test.tssrc/__tests__/clientFixtures.tssrc/__tests__/config.test.tssrc/__tests__/httpServer.test.tssrc/__tests__/integration.test.tssrc/__tests__/jsonSchema.test.tssrc/__tests__/logger.test.tssrc/__tests__/mcpServer.test.tssrc/__tests__/mcpTelemetry.test.tssrc/__tests__/redashClient.test.tssrc/__tests__/setupEnv.tssrc/__tests__/startup.test.tssrc/__tests__/telemetryConfig.test.tssrc/bigQuerySchema.tssrc/chartVisualization.tssrc/cli.tssrc/config.tssrc/httpServer.tssrc/index.tssrc/jsonSchema.tssrc/logger.tssrc/mcpProtocol.tssrc/mcpTelemetry.tssrc/opentelemetry-semconv-incubating.d.tssrc/packageInfo.tssrc/redashClient.tssrc/startup.tssrc/telemetry.tssrc/telemetryConfig.tssrc/utils.tstest/fixtures/otel-app.mjstest/otel-export.test.mjstsconfig.json
💤 Files with no reviewable changes (2)
- src/jsonSchema.ts
- src/tests/jsonSchema.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .npmrc
- src/tests/bigQuerySchema.test.ts
- src/bigQuerySchema.ts
| with: | ||
| run_install: false | ||
|
|
||
| - uses: actions/setup-node@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect workflow triggers to assess cache-scope exposure across privileged and PR-triggered jobs.
rg -n -A5 '^on:' .github/workflows/test.yml .github/workflows/tagpr.yml .github/workflows/release.ymlRepository: kahirokunn/redash-mcp
Length of output: 919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workflow files:"
git ls-files .github/workflows
echo
echo "release.yml relevant sections:"
cat -n .github/workflows/release.yml | sed -n '1,220p'
echo
echo "test.yml relevant sections:"
cat -n .github/workflows/test.yml | sed -n '1,240p'
echo
echo "tagpr.yml relevant sections:"
cat -n .github/tagpr.yml | sed -n '1,220p' 2>/dev/null || cat -n .github/workflows/tagpr.yml | sed -n '1,220p'
echo
echo "Search for cache, setup-node, permissions, pull_request in workflows:"
rg -n "permissions:\n |pull_request|setup-node|pnpm|cache|artifact|setup-node@v6|id-token|contents:|packages:|[sS]ecure|docker|npm publish|pnpm-cache|setup-pnpm" .github/workflowsRepository: kahirokunn/redash-mcp
Length of output: 10092
🌐 Web query:
GitHub Actions setup-node cache pull_request workflows cache scope restore cache cache-save docs
💡 Result:
To manage caching in pull request workflows using actions/setup-node, it is important to understand the default behavior, cache scoping, and how to implement "restore-only" (read-only) mode to avoid potential cache poisoning risks [1][2][3]. Default Behavior and Scoping GitHub Actions caches are scoped by branch [4]. A workflow triggered by a pull request can restore caches from its own branch, the repository's default branch, and the base branch (including base branches of forks) [3]. However, when a cache is created by a pull request, it is saved for the merge ref (refs/pull/.../merge) [3]. Consequently, this cache has a limited scope and cannot be accessed by the base branch or other pull requests targeting that same base branch; it is only available to re-runs of that specific pull request [3]. Implementing Restore-Only (Read-Only) Caching In pull request workflows, particularly those triggered from forks or untrusted sources, you may want to restore existing dependencies from the cache without allowing the workflow to write new entries back to the cache [2]. This prevents cache poisoning, where a malicious pull request could overwrite a valid cache with tainted packages [1][2]. To achieve this, use the cache-write input (available in recent versions of actions/setup-node) [2]. This allows you to selectively disable the post-step cache save [2]. Example Configuration: - uses: actions/setup-node@v4 with: node-version: 20 cache: npm # Only write to cache on non-PR events to prevent cache poisoning cache-write: ${{ github.event_name!= 'pull_request' }} If you are using an older version of the action that does not support the cache-write input, you can achieve similar results by using the actions/cache/restore action directly instead of the built-in setup-node caching [5]. Important Considerations: - Automatic Caching: For npm, actions/setup-node automatically enables caching if the packageManager or devEngines.packageManager field is set to npm in package.json [6][7]. For workflows requiring high security, you should explicitly disable this by setting package-manager-cache: false if you do not need it or if you intend to manage cache steps manually [1][7]. - Storage and Performance: Utilizing restore-only mode reduces cache writes, which helps manage storage quotas and keeps the cache clean in read-only workflows [1][2]. Sources: [1], [2], [3]
Citations:
- 1: https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md
- 2: Add cache-write input for read-only cache mode actions/setup-node#1508
- 3: https://docs.github.com/en/actions/reference/workflows-and-actions/dependency-caching
- 4: https://runs-on.com/github-actions/caching-dependencies/
- 5: Add example for restore-only cache in documentation actions/setup-node#1419
- 6: https://github.com/actions/setup-node
- 7: https://github.com/actions/setup-node/tree/refs/heads/main
🌐 Web query:
GitHub Actions setup-node cache pull_request default branch cache branch cache scope actions/setup-node
💡 Result:
GitHub Actions setup-node uses the actions/cache utility under the hood to manage dependency caching [1][2][3]. The following technical details explain how cache scoping and pull request access behave: ### Cache Scope and Access Restrictions GitHub Actions caches are scoped to the branch and the workflow key [4]. To ensure security and prevent cache poisoning, GitHub imposes specific access restrictions: * Default Branch: Workflows running on the default branch (e.g., main) can create, update, and restore caches [4][5]. * Pull Requests: Workflows triggered by a pull request can generally restore caches created in the current branch or the base branch (including the default branch) [5][6]. However, they often have read-only access to these caches and may be restricted from creating or saving new cache entries, especially if the pull request originates from a fork [4]. ### Best Practices for Pull Requests Because of these restrictions, caching is most effective when a "base" cache already exists on the default branch [5]. Child branches and pull requests can then pull from that existing cache to speed up dependency installation [5]. To manage caching effectively in pull requests and avoid security risks, consider these approaches: * Read-Only Caching: To prevent untrusted pull requests from writing to your cache (which could lead to cache poisoning), you can use the cache-write input (available in recent versions of setup-node) [7]. By setting cache-write: ${{ github.event_name != 'pull_request' }}, you allow pull requests to restore from the cache without updating it [7]. * Automatic Caching: By default, setup-node enables automatic caching for npm when a packageManager field is detected in your package.json [1][2][3]. You can control this behavior using the package-manager-cache input (set to true by default) [1][8]. * Security: For workflows with elevated privileges or access to sensitive information, it is recommended to set package-manager-cache: false if caching is not strictly required, to avoid exposing credentials or sensitive data through the cache [9][2]. ### Summary of setup-node Caching Inputs * cache: Used to explicitly enable caching (e.g., npm, yarn, pnpm) [1]. * package-manager-cache: A boolean to enable/disable automatic npm caching based on package.json [1][8]. * cache-write: A boolean used to allow restore-only behavior, protecting the cache from modification by untrusted pull requests [7].
Citations:
- 1: https://github.com/actions/setup-node
- 2: https://github.com/actions/setup-node/blob/v6/README.md
- 3: https://github.com/actions/setup-node/tree/refs/heads/main
- 4: https://github.com/actions/cache
- 5: can NOT find cache actions/setup-node#539
- 6: github action cache not working actions/setup-node#819
- 7: Add cache-write input for read-only cache mode actions/setup-node#1508
- 8: Enhance caching in setup-node with automatic package manager detection actions/setup-node#1348
- 9: https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md
Disable setup-node cache writes in pull_request jobs.
.github/workflows/test.yml enables pnpm caching on pull_request, so a PR-targeted workflow can add entries to the cache key reused by the privileged .github/workflows/release.yml job. Add cache-write: ${{ github.event_name != 'pull_request' }} to actions/setup-node@v6 in .github/workflows/test.yml and use the same read-only check for any additional cache writes there.
🧰 Tools
🪛 zizmor (1.28.0)
[error] 32-32: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml at line 32, Update the actions/setup-node@v6
configuration in the pull-request test workflow to set cache-write based on
whether github.event_name is not pull_request, and apply the same read-only
condition to any other cache-writing steps in that workflow.
Source: Linters/SAST tools
| - uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: '24' | ||
| node-version: '20' | ||
| registry-url: 'https://registry.npmjs.org' | ||
| package-manager-cache: false | ||
| cache: 'pnpm' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does npm Trusted Publishing require Node 22.14.0 or later, and does it work with Node 20?
💡 Result:
Yes, npm Trusted Publishing requires Node.js 22.14.0 or higher [1][2][3]. While the requirement is officially documented as Node 22.14.0 or higher [1], the core functional requirement is actually the version of the npm CLI [4][5][6]. Specifically, Trusted Publishing requires npm CLI version 11.5.1 or later to perform the necessary OpenID Connect (OIDC) token exchange [1][5][7][8]. Because older versions of Node.js (including Node 20) bundle versions of the npm CLI that are earlier than 11.5.1, they do not support Trusted Publishing out of the box [4][6][8]. If you attempt to use Trusted Publishing with Node 20 or an older version of npm, the CLI will not recognize the OIDC flow, will fail to authenticate, and will likely result in an error such as ENEEDAUTH [4][6][8]. To use Trusted Publishing while running on an older Node.js version, you must explicitly upgrade the npm CLI to at least version 11.5.1 as a step in your CI/CD workflow before running the publish command [4][5][6][2]. This can be achieved with a command such as: npm install -g npm@latest [5][2][8] Using a newer Node.js version (like Node 24 or later) is often the simplest path, as these versions include a sufficiently modern npm CLI by default [4][6].
Citations:
- 1: https://docs.npmjs.com/trusted-publishers/
- 2: https://github.com/OpenHands/typescript-client/blob/734e996d0f34501966fee1529d5ad4885260397b/.github/workflows/npm-publish.yml
- 3: https://github.com/joemonster/zfork-ai-writing-guide/blob/main/.github/workflows/npm-publish.yml
- 4: https://jescalada.com/blog/2026-02-01-switching-npm-trusted-publishers-mind-node-version/
- 5: https://philna.sh/blog/2026/01/28/trusted-publishing-npm/
- 6: https://stackoverflow.com/questions/79952803/why-am-i-getting-a-404-when-i-use-trusted-publishing-to-publish-from-github-acti
- 7: https://github.blog/changelog/2025-07-31-npm-trusted-publishing-with-oidc-is-generally-available/
- 8: ZRktty/typelock@29683a4
🌐 Web query:
npm release workflow setup-node registry-url _authToken NODE_AUTH_TOKEN empty
💡 Result:
When using the actions/setup-node GitHub Action with the registry-url input, the action automatically creates or updates an.npmrc file in the runner environment [1][2][3]. Specifically, it writes a line in the format //registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN} [4][5][3]. This mechanism creates a critical conflict with npm Trusted Publishing (OIDC) workflows [4][5]. Here is how the conflict occurs and how to resolve it: 1. The Conflict: When you configure Trusted Publishing, you intentionally omit a secret for npm authentication because npm CLI is expected to perform an OIDC token exchange instead [4][6][7]. However, because the.npmrc contains the _authToken=${NODE_AUTH_TOKEN} line, the npm CLI detects this configuration [4][5]. If the NODE_AUTH_TOKEN environment variable is unset or empty, the placeholder expands to an empty string [5][6][7]. The npm CLI interprets this empty string as "auth is configured" (but with invalid credentials) and attempts a classic publish, failing with ENEEDAUTH or E404 instead of falling back to the OIDC flow [4][5][8][7]. 2. Important Requirement: For OIDC to work, the NODE_AUTH_TOKEN environment variable must not be set at all [6][7]. Even if it is set to an empty string, npm will prioritize the (failed) token attempt over OIDC [8][6][7]. 3. Recommended Solutions: - Remove the registry-url input: If you do not require custom.npmrc configuration from setup-node, remove the registry-url input entirely to prevent the action from writing the _authToken line [4][5]. - Clean the.npmrc: If you must use registry-url, you can manually strip the _authToken line after the setup-node step completes but before running npm publish [4][5]: run: | npmrc="${NPM_CONFIG_USERCONFIG:-$HOME/.npmrc}" sed -i '/_authToken/d' "$npmrc" - Ensure Node.js Version Compatibility: Ensure you are using a modern Node.js version (Node.js 24+ is recommended) that supports the necessary npm CLI versions for native OIDC Trusted Publishing [8][7]. Recent updates to actions/setup-node have improved this behavior by removing the previous default of exporting a dummy token, which helps prevent unintended token injection [9][10]. However, if the variable NODE_AUTH_TOKEN is explicitly set in your workflow environment, it will still interfere with OIDC [9][8]. Ensure no env: block in your publish step passes a NODE_AUTH_TOKEN secret [11][7].
Citations:
- 1: https://github.com/actions/setup-node/blob/master/src/authutil.ts
- 2: https://docs.github.com/en/enterprise-server@3.20/actions/tutorials/build-and-test-code/nodejs
- 3: https://docs.github.com/en/enterprise-server@3.19/actions/tutorials/build-and-test-code/nodejs
- 4: registry-url writes _authToken line that breaks npm Trusted Publisher OIDC when no NODE_AUTH_TOKEN is set actions/setup-node#1551
- 5: actions/setup-node registry-url interferes with OIDC trigger — example workflow fails with ENEEDAUTH when no NODE_AUTH_TOKEN npm/documentation#1960
- 6: https://dev.to/fernforge/why-your-npm-trusted-publishing-setup-404s-at-release-time-six-causes-checked-before-you-push-361g
- 7: https://jescalada.com/blog/2026-02-01-switching-npm-trusted-publishers-mind-node-version/
- 8: Don't default NPM_AUTH_TOKEN to support NPM OIDC actions/setup-node#1440
- 9: Remove dummy NODE_AUTH_TOKEN export actions/setup-node#1558
- 10: https://github.com/actions/setup-node?tab=readme-ov-file
- 11: https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== release workflow relevant lines =="
sed -n '1,45p;170,185p' .github/workflows/release.yml
echo
echo "== pnpm config and node/engine package references =="
if [ -f package.json ]; then
python3 - <<'PY'
import json
from pathlib import Path
p=Path('package.json')
data=json.loads(p.read_text())
print("engines:", data.get("engines"))
for k in sorted(data):
if k in ("packageManager", "engines"):
print(f"{k}: {data[k]}")
PY
fi
git ls-files | rg '(^|/)(pnpm-lock.yaml|package-lock\.json|yarn\.lock|npm-shrinkwrap\.json)$|(^|/)package\.json$' | while read -r f; do
echo "-- $f"
rg -n '"pnpm"|"node"|"engines"|"packageManager"|npm@latest|setup-node|registry-url|NODE_AUTH_TOKEN|NPM_TOKEN|npm publish|permissions:' "$f" || true
done
echo
echo "== npm setup/auth related action references =="
rg -n 'actions/setup-node|registry-url|npm publish|NODE_AUTH_TOKEN|NPM_TOKEN|permissions:\s*id-token|pnpm install|npm install -g npm|npm ci' .github/workflows || trueRepository: kahirokunn/redash-mcp
Length of output: 2868
🌐 Web query:
npmjs trusted publishers npm CLI version 11.5.1 documentation
💡 Result:
Trusted publishing is a security feature for npm that allows you to publish packages from your CI/CD workflows using OpenID Connect (OIDC) authentication [1][2]. This eliminates the need for long-lived, account-level npm access tokens [1][3]. Requirements To use trusted publishing, you must meet the following requirements: - npm CLI: Version 11.5.1 or later [1][3][4]. - Node.js: Version 22.14.0 or higher [1][2]. - CI/CD Provider: Currently supports cloud-hosted runners for GitHub Actions, GitLab CI/CD, and CircleCI [1]. How it Works Trusted publishing creates a cryptographic trust relationship between the npm registry and your CI/CD provider [1][3]. When you configure this relationship, the npm CLI automatically detects the OIDC environment in your CI/CD runner, exchanges the provider's OIDC token for a short-lived, package-specific registry token, and performs the publish operation [1][2][5]. Key Features - Enhanced Security: Uses short-lived, workflow-specific credentials that cannot be exfiltrated or reused like traditional tokens [3][2][5]. - Automatic Provenance: When using trusted publishing, npm automatically generates and publishes provenance attestations for your package, meaning the --provenance flag is generally not required [6][3][2]. - Granular Control: You can specify strict constraints (e.g., repository, workflow file path, branch, or environment) for which workflows are authorized to publish a package [5][7]. Configuration 1. npm Registry: Navigate to your package settings on npmjs.com (typically located at https://www.npmjs.com/package//access) to configure the trusted publisher [4][5][7]. You must select your CI/CD provider and specify the required identity details (e.g., repository owner, repository name, and workflow filename) [1][3][7]. 2. CI/CD Workflow: Update your workflow file to grant the necessary OIDC permissions (e.g., id-token: write for GitHub Actions) [4][5][7]. 3. npm CLI Command: You can also manage these relationships from the command line using the npm trust command, which serves as the CLI equivalent to the website configuration [8][9]. Use npm trust --help to see provider-specific options and configuration flags [8]. Important Note Trusted publishing is intended for the npm publish process [2]. Installing private packages still requires traditional authentication tokens [2]. If you encounter issues during setup, ensure your package.json contains a valid repository field that matches your CI/CD provider's repository settings exactly [4][7].
Citations:
- 1: https://docs.npmjs.com/trusted-publishers/
- 2: https://deepwiki.com/npm/documentation/4.4.4-trusted-publishers
- 3: https://github.blog/changelog/2025-07-31-npm-trusted-publishing-with-oidc-is-generally-available/
- 4: https://philna.sh/blog/2026/01/28/trusted-publishing-npm/
- 5: https://safeguard.sh/resources/blog/npm-trusted-publishing-walkthrough-2026
- 6: https://docs.npmjs.com/generating-provenance-statements/
- 7: https://leechael.org/posts/2025/npm-trusted-publishers-the-complete-guide/
- 8: https://docs.npmjs.com/cli/v11/commands/npm-trust/
- 9: https://manpages.debian.org/unstable/npm/npm-trust.1.en.html
🌐 Web query:
"setup-node" "registry-url" "_authToken=${NODE_AUTH_TOKEN}"
💡 Result:
The registry-url input for the actions/setup-node GitHub Action is used to configure authentication for private npm registries or package publishing [1][2][3]. When you provide a registry-url to setup-node, the action automatically generates an .npmrc file on the runner [2][4]. This file includes a line that instructs npm to use an environment variable for authentication, formatted as follows [2][3][5]: //registry.example.org/:_authToken=${NODE_AUTH_TOKEN} In this configuration: 1. //registry.example.org/ is replaced with the registry URL provided to registry-url (minus the protocol) [4]. 2. ${NODE_AUTH_TOKEN} is a literal placeholder that instructs npm to look for an environment variable named NODE_AUTH_TOKEN at runtime to provide the actual authentication token [2][5][6]. To use this successfully, you must ensure that the NODE_AUTH_TOKEN environment variable is set in the steps where you run npm commands (such as npm install or npm publish) [2][3][6]. This is typically done using GitHub Secrets to keep the token secure [2][3]: - uses: actions/setup-node@v7 with: registry-url: 'https://registry.npmjs.org' - run: npm publish env: NODE_AUTH_TOKEN: ${{ secrets.YOUR_NPM_TOKEN }} Important Considerations: - Environment Variable Name: The environment variable must be named NODE_AUTH_TOKEN because the .npmrc file generated by setup-node is hardcoded to look for that specific variable name [5]. - OIDC/Trusted Publishing: If you are using npm Trusted Publishing (OIDC) to publish packages, you may not need or want this token-based configuration [7]. In some cases, if registry-url is specified without a corresponding NODE_AUTH_TOKEN secret, the generated _authToken line can interfere with OIDC authentication [7]. In such environments, you may need to manually remove the line or use alternative configuration methods to allow OIDC to proceed [7].
Citations:
- 1: https://github.com/actions/setup-node
- 2: https://docs.github.com/en/actions/tutorials/publish-packages/publish-nodejs-packages
- 3: https://docs.github.com/en/actions/tutorials/build-and-test-code/nodejs
- 4: registry-url ignored completely actions/setup-node#926
- 5: https://stackoverflow.com/questions/75766122/whats-the-difference-between-node-auth-token-and-npm-auth-token
- 6: Fixed auth GPR actions/setup-node#64
- 7: registry-url writes _authToken line that breaks npm Trusted Publisher OIDC when no NODE_AUTH_TOKEN is set actions/setup-node#1551
Avoid using OIDC Trusted Publishing with Node 20.
Trusted Publishing requires npm CLI 11.5.1+ and Node.js 22.14.0+, which this workflow does not provide. If this job relies on permissions: id-token: write, upgrade Node to at least 22.14.0 and run npm install -g npm@latest before npm publish; remove registry-url or clean the generated _authToken config so OIDC can be selected. If manual token publishing is intended instead, pass NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} to the publish step.
🧰 Tools
🪛 zizmor (1.28.0)
[error] 32-32: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 32 - 36, Update the release
workflow’s Node setup and publish configuration: for OIDC Trusted Publishing,
use Node.js 22.14.0 or newer, install npm 11.5.1 or newer before publishing, and
remove registry-url or clean the generated _authToken configuration so OIDC is
selected; otherwise configure the publish step to use NODE_AUTH_TOKEN from
secrets.NPM_TOKEN for manual token publishing.
| "scripts": { | ||
| "build": "npx tsc && chmod +x dist/index.js dist/cli.js", | ||
| "clean": "node --eval \"require('node:fs').rmSync('dist', { recursive: true, force: true })\"", | ||
| "build": "pnpm run clean && tsc && chmod +x dist/cli.js", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package snippets =="
sed -n '1,90p' package.json
echo
echo "== dependency references =="
rg -n '"(build|inspector:headless|cross-env|pnpm)"|chmod|BROWSER=none' -S . --glob '!node_modules' --glob '!dist' --glob '!coverage' | head -200Repository: kahirokunn/redash-mcp
Length of output: 3524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== devDependencies full =="
python3 - <<'PY'
import json
from pathlib import Path
pkg=json.loads(Path("package.json").read_text())
dev=pkg.get("devDependencies", {})
for k in sorted(dev):
print(k, "=>", dev[k])
PY
echo
echo "== lockfile summary =="
if [ -f pnpm-lock.yaml ]; then
rg -n "^[[:space:]]+(`@eslint/js`|`@jest/environment`|cross-env|typescript|pnpm)" pnpm-lock.yaml | head -200 || true
else
echo "no pnpm-lock.yaml"
fi
echo
echo "== documentation/windows guidance =="
rg -n "windows|win32|cmd|powershell|shell|POSIX|script|chmod|BROWSER|cross-env" -S . --glob '!node_modules' --glob '!dist' --glob '!coverage' | sed -n '1,200p'Repository: kahirokunn/redash-mcp
Length of output: 17314
🌐 Web query:
pnpm cli run script commands POSIX features fail on non POSIX systems except Windows with shell emulator
💡 Result:
By default, pnpm executes scripts using the system's native shell [1][2]. On POSIX-compliant systems (like Linux and macOS), this native shell supports standard POSIX features (such as environment variable assignment like NODE_ENV=test and command chaining) [1][3]. However, on non-POSIX-compliant systems, such as Windows (outside of specialized environments like Git Bash or WSL), these native shells do not support such features, causing scripts that rely on them to fail [1][2]. To address this, pnpm provides an opt-in shellEmulator feature [1][4]. When enabled by setting shell-emulator=true in your.npmrc file, pnpm utilizes a JavaScript-based, bash-like shell implementation (powered by @yarnpkg/shell) to execute scripts [1][5][6]. This emulator allows scripts that use common POSIX syntax to run consistently across all platforms, including Windows, without requiring additional dependencies like cross-env [1][6]. It is important to note that the shellEmulator is not a perfect POSIX implementation and may encounter edge cases or bugs [7][5]. For instance, certain built-in commands like cd have historically faced compatibility issues when running within the emulator on Windows [7]. Consequently, while the shellEmulator significantly improves cross-platform script reliability, it is an opt-in feature rather than the default behavior [1][2].
Citations:
- 1: https://pnpm.io/cli/run
- 2: https://pnpm.io/10.x/cli/run
- 3: https://pnpm.io/next/cli/run
- 4: https://www.bookstack.cn/read/pnpm-7.x-en/370e18392582eebf.md
- 5: Use Yarn's bash-like shell for running scripts pnpm/pnpm#2621
- 6: https://www.petermekhaeil.com/til/pnpm-shell-emulator/
- 7:
cdcommand failed on Windows withshell-emulator=truein.npmrcpnpm/pnpm#7838
Make the package scripts POSIX-independent for Windows.
build uses chmod, and inspector:headless uses BROWSER=none; both use POSIX shell syntax that fails under the default Windows shell. Use a Node-based executable-step for chmod and a cross-platform env wrapper or shell-emulator for BROWSER.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 37, Update the package.json build script to replace the
POSIX chmod command with a Node-based executable-step, and update
inspector:headless to set BROWSER through the project’s cross-platform
environment wrapper or shell emulator instead of inline POSIX syntax. Preserve
the existing command order and behavior while making both scripts work under
Windows’ default shell.
| import axios from 'axios'; | ||
| import { getRedashClient, RedashClient } from '../redashClient.js'; | ||
| import { toolDefinitions } from '../index.js'; | ||
| import { logger } from '../logger.js'; | ||
| import { jest } from '@jest/globals'; | ||
|
|
||
| // Mock axios to avoid real API calls | ||
| jest.mock('axios'); | ||
|
|
||
| describe('MCP Server Integration', () => { | ||
| beforeEach(() => { | ||
| // Set up environment variables for testing | ||
| process.env.REDASH_URL = 'https://redash.example.com'; | ||
| process.env.REDASH_API_KEY = 'test-api-key'; | ||
| }); | ||
| function getToolInputSchema(name: string) { | ||
| const tool = toolDefinitions.find((definition) => definition.name === name); | ||
| if (!tool) { | ||
| throw new Error(`Tool ${name} is not registered`); | ||
| } | ||
|
|
||
| return tool.inputSchema; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the toolDefinitions inputSchema shape and the Jest module configuration.
set -euo pipefail
echo '=== toolDefinitions declaration ==='
rg -nP --type=ts -C 10 'export const toolDefinitions' src/index.ts || true
echo '=== a few inputSchema entries ==='
rg -nP --type=ts -C 2 '\binputSchema\s*:' src/index.ts | head -60
echo '=== jest configuration ==='
fd -t f 'jest.config' --exec cat -n {} \;
echo '=== ESM/preset indicators in package.json ==='
fd -t f '^package.json$' -d 1 --exec jq '{type, jest, scripts}' {} \;Repository: kahirokunn/redash-mcp
Length of output: 3630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== defineTool definition ==='
rg -nP --type=ts -C 8 '\bfunction defineTool\b|\bconst defineTool\b|interface .*ToolDefinition|type .*ToolDefinition' src/index.ts
echo '=== schema conversion implementation ==='
rg -nP --type=ts -C 10 '\bfunction cacheJsonSchemaConversion\b|\bconst cacheJsonSchemaConversion\b|cacheJsonSchemaConversion\s*=' src/index.ts
echo '=== package dependencies relevant to zod/ts-jest/axios ==='
fd -t f '^package.json$' -d 1 --exec jq '{dependencies, devDependencies | with_entries(select(.key | test("^(jest|ts-jest|axios|zod)|`@modelcontextprotocol`)"))}' {} \;
echo '=== lockfile package declarations ==='
fd -t f '(^pnpm-lock.yaml$|^package-lock.json$|^yarn.lock$)' -d 1 -p 'pnpm-lock.yaml' -p 'package-lock.json' -p 'yarn.lock' --exec sh -c 'echo "--- $1"; rg -n "ts-jest|jest-mock|axios|zod" "$1" | head -80' sh {} \;
echo '=== imports in integration test and setupEnv ==='
cat -n src/__tests__/integration.test.ts | sed -n '1,80p'
if [ -f src/__tests__/setupEnv.ts ]; then cat -n src/__tests__/setupEnv.ts | sed -n '1,80p'; fiRepository: kahirokunn/redash-mcp
Length of output: 1896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== package dependencies ==='
fd -t f '^package.json$' -d 1 --exec jq -r '.dependencies + .devDependencies // {} | to_entries | select(.key | test("^(jest|ts-jest|axios|zod)|`@modelcontextprotocol`")) | .key + "=" + .value' {} \;
echo '=== lockfile package declarations ==='
fd -t f '(^pnpm-lock.yaml$|^package-lock.json$|^yarn.lock$)' -d 1 -p 'pnpm-lock.yaml' -p 'package-lock.json' -p 'yarn.lock' --exec sh -c 'echo "--- $1"; rg -n "ts-jest|jest-mock|axios|zod" "$1" | head -80' sh {} \;
echo '=== imports in integration test and setupEnv ==='
cat -n src/__tests__/integration.test.ts | sed -n '1,80p'
if [ -f src/__tests__/setupEnv.ts ]; then cat -n src/__tests__/setupEnv.ts | sed -n '1,80p'; fiRepository: kahirokunn/redash-mcp
Length of output: 492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== package.json snippet ==='
fd -t f '^package.json$' -d 1 --exec jq '{type, dependencies, devDependencies}' {} \;
echo '=== lockfile package declarations ==='
fd -t f '(^pnpm-lock.yaml$|^package-lock.json$|^yarn.lock$)' -d 1 -p 'pnpm-lock.yaml' -p 'package-lock.json' -p 'yarn.lock' --exec sh -c 'echo "--- $1"; rg -n "ts-jest|jest-mock|axios|zod" "$1" | head -80' sh {} \;
echo '=== imports in integration test and setupEnv ==='
cat -n src/__tests__/integration.test.ts | sed -n '1,80p'
if [ -f src/__tests__/setupEnv.ts ]; then cat -n src/__tests__/setupEnv.ts | sed -n '1,80p'; fiRepository: kahirokunn/redash-mcp
Length of output: 2212
🌐 Web query:
@jest/globals jest.mock ESM hoisting order static imports documentation
💡 Result:
When using ECMAScript Modules (ESM) in Jest, the standard hoisting mechanism used for CommonJS (CJS) does not apply [1][2]. In CJS environments, Jest automatically hoists jest.mock calls to the top of the file, ensuring mocks are defined before any modules are imported [2][3]. However, because the ESM specification requires static import statements to be evaluated before any code is executed, this hoisting is not possible with static imports [1][4]. Consequently, if you use static imports in an ESM-enabled Jest test, the module will be loaded before the jest.mock call can execute, resulting in the original module being used instead of the mock [5][6]. To work around this limitation, you must avoid static imports for the modules you intend to mock [5][6]. Instead, you should: 1. Remove static imports of the modules you wish to mock and any modules that depend on them [5][6]. 2. Use dynamic import calls after your mock definitions to load the code under test [1][5]. For mocking ESM files, Jest provides the experimental API jest.unstable_mockModule [1][7]. This function is specifically designed for ESM and requires a factory function [1][4]. It operates similarly to jest.mock but must be called before the dynamic import of the module to ensure the mock is registered in the module graph [1][5]. Example pattern: import {jest} from '@jest/globals'; // Register the mock using unstable_mockModule for ESM jest.unstable_mockModule('some-module', => ({ someFunction: jest.fn, })); // Use dynamic import after the mock is defined const {someFunction} = await import('some-module'); const myModule = await import('./my-module'); // Alternatively, for CJS modules in an ESM test, // you may still use jest.mock combined with dynamic imports [1][4] jest.mock('some-cjs-module', => ({...})); const {exported} = await import('./module-under-test.cjs');
Citations:
- 1: https://jestjs.io/docs/ecmascript-modules
- 2: https://jestjs.io/docs/manual-mocks
- 3: https://github.com/facebook/jest/blob/master/docs/ManualMocks.md
- 4: https://github.com/jestjs/jest/blob/main/docs/ECMAScriptModules.md
- 5: [Bug]: jest.mock not working with ESM support activated jestjs/jest#13135
- 6: jest.mock does not mock an ES module without Babel jestjs/jest#10025
- 7: https://jestjs.io/docs/next/ecmascript-modules
Use the ESM mock API for static axios imports.
jest.mock('axios') is registered after import axios from 'axios', and this project runs tests as ESM. In that runtime, static import evaluation can precede jest.mock, so the statically imported axios client can bypass the mock. Replace this with jest.unstable_mockModule('axios', ...) and load the tested modules dynamically after the mock is registered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/integration.test.ts` around lines 8 - 24, The static axios
import bypasses Jest’s mock in the ESM test runtime. Replace jest.mock in the
integration test with jest.unstable_mockModule('axios', ...) and dynamically
import axios and the tested modules only after registering the mock, updating
references such as getRedashClient, RedashClient, toolDefinitions, and logger
accordingly.
| // The stateless HTTP transport builds a fresh McpServer per request, and | ||
| // registerTool eagerly converts each input schema to JSON Schema. The schemas | ||
| // are immutable module-level constants, so memoize the conversion per schema. | ||
| function cacheJsonSchemaConversion<T extends ZodRawShape>(schema: ZodObject<T>): ZodObject<T> { | ||
| const std = schema["~standard"] as { jsonSchema?: JsonSchemaConverter }; | ||
| const converter = std.jsonSchema; | ||
| if (!converter) { | ||
| return schema; | ||
| } | ||
|
|
||
| const cachedByTarget = new Map<string, Record<string, unknown>>(); | ||
| std.jsonSchema = { | ||
| input: (options) => { | ||
| let converted = cachedByTarget.get(options.target); | ||
| if (!converted) { | ||
| converted = converter.input(options); | ||
| cachedByTarget.set(options.target, converted); | ||
| } | ||
| return converted; | ||
| }, | ||
| output: (options) => converter.output(options), | ||
| }; | ||
|
|
||
| return schema; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
zod 4 "~standard" jsonSchema property Standard Schema JSON Schema conversion
💡 Result:
In Zod 4, JSON Schema conversion is natively supported and integrated with the Standard Schema specification [1][2]. ### Standard Schema JSON Schema Conversion Zod 4 schemas implement the Standard JSON Schema V1 interface [2][3]. This allows Zod schemas to expose JSON Schema generation methods directly through the ~standard.jsonSchema property [2]. Because this property is non-enumerable, it avoids polluting the schema object [2]. You can access these methods to generate JSON Schema representations for both input and output types: * Input Schema: schema['~standard'].jsonSchema.input({ target: '...' }) [2][4] * Output Schema: schema['~standard'].jsonSchema.output({ target: '...' }) [2][4] ### Supported Targets The conversion supports various JSON Schema versions and formats [1][5]: * draft-2020-12 (Default) [1][6] * draft-07 [1][6] * draft-04 [1][6] * openapi-3.0 [1][6] ### Additional Methods In addition to the Standard Schema integration, Zod 4 provides a direct instance method for convenience [1][2]: * schema.toJSONSchema(params): This function accepts similar parameters (e.g., target, metadata, cycles, reused) to control the conversion process [1][5][6]. The Standard JSON Schema specification was designed to ensure that type information is preserved during conversion between different validation libraries [3]. By using the ~standard interface, your code remains interoperable with other tools that support the Standard Schema spec [3][7]. Top results: [1][2][3]
Citations:
- 1: https://zod.dev/json-schema
- 2: Add
.toJSONSchema()method colinhacks/zod#5477 - 3: https://standardschema.dev/json-schema
- 4: https://cdn.jsdelivr.net/npm/zod@4.3.6/src/v4/core/standard-schema.ts
- 5: https://github.com/colinhacks/zod/blob/main/packages/zod/src/v4/core/to-json-schema.ts
- 6: https://cdn.jsdelivr.net/npm/zod@4.3.6/src/v4/core/to-json-schema.ts
- 7: https://standardschema.dev/schema
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package manifests snippets =="
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock bun.lockb; do
if [ -f "$f" ]; then
echo "--- $f"
if [ "$f" = "package.json" ]; then
node -e 'const p=require("./package.json"); console.log(JSON.stringify({dependencies:p.dependencies,devDependencies:p.devDependencies,engines:p.engines}, null, 2))'
else
rg -n "zod|`@modelcontextprotocol/server`|`@modelcontextprotocol/sdk`|`@modelcontextprotocol`" "$f" | head -80 || true
fi
fi
done
echo "== relevant imports/usages in src/index.ts =="
wc -l src/index.ts || true
sed -n '1,110p' src/index.ts
echo "== repo-wide JSON schema conversion references =="
rg -n "~standard|jsonSchema|toJSONSchema|cacheJsonSchemaConversion|emptyInputSchema" . -g '!node_modules' -g '!dist' -g '!build' | head -200Repository: kahirokunn/redash-mcp
Length of output: 7743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test files =="
git ls-files | rg '(^test/|^src/__tests__|^.*\.test\.ts$|^.*\.spec\.ts$|jest|vitest|playwright)' | head -200
echo "== zod jsonSchema behavior probe if node_modules is available =="
if [ -d node_modules/zod ]; then
node - <<'JS'
const { z } = require('zod');
const obj = z.object({ a: z.string() });
console.log({
hasStandard: obj.hasOwnProperty('~standard') || '__proto__' in {} && obj['~standard'] != null,
hasJsonSchema: obj['~standard'] && !!obj['~standard'].jsonSchema,
converterType: typeof (obj['~standard'] && obj['~standard'].jsonSchema),
inputCallWorks: (() => {
try { obj['~standard'].jsonSchema.input({ target: 'draft-2020-12' }); return true; } catch { return false; }
})()
});
JS
else
echo "node_modules/zod not available"
fi
echo "== lock entries around zod and mcp server =="
sed -n '14,32p' pnpm-lock.yaml
sed -n '94,108p' pnpm-lock.yaml
sed -n '1790,1802p' pnpm-lock.yaml
sed -n '2226,2234p' pnpm-lock.yamlRepository: kahirokunn/redash-mcp
Length of output: 2748
🌐 Web query:
@modelcontextprotocol/server 2.0.0 JSON Schema conversion defineTool inputSchema toStandardSchema
💡 Result:
In version 2.0.0 of the Model Context Protocol (MCP) TypeScript SDK, the defineTool (used via server.registerTool) and associated schema handling have evolved to embrace the Standard Schema specification [1][2][3]. Key points regarding schema conversion and usage in MCP SDK 2.0.0: 1. Standard Schema Support: Tool and prompt registration now natively accept any schema library that implements the Standard Schema specification (e.g., Zod v4, ArkType, Valibot) [1][2][3]. This replaces the previous Zod-specific implementation [1][4]. 2. Schema Requirements: Registered schemas must implement StandardSchemaWithJSON, which provides both ~standard.validate (for runtime argument validation) and ~standard.jsonSchema (to advertise the tool's interface to clients) [1][3]. 3. Conversion Adapters: - For raw JSON Schema objects, the SDK exports fromJsonSchema to wrap them as a validator [5][2][4]. - If using schema libraries that do not implement the ~standard.jsonSchema interface natively (such as older Zod versions), you should either upgrade the library or use an appropriate converter/wrapper [6][3]. - While some external tools (like those in the Mastra ecosystem) provide toStandardSchema utilities for compatibility [7][8][9], the MCP SDK itself provides standardSchemaToJsonSchema for internal use [4][3]. 4. Deprecation: The "raw-shape" overload (passing a plain object like { field: z.string() } directly) is deprecated [2][10][11]. You should pass a full schema object (e.g., z.object({...})) instead [2][3]. 5. MCP Specification Constraints: MCP requires tool input/output schemas to describe objects at their root [3][12]. The SDK enforces this and will throw an error if the schema describes a different type (e.g., a bare string or number) [6][3]. If you are encountering toStandardSchema, note that this is often a helper utility in third-party libraries (like mastra-ai) rather than a core method exported directly for users by the official @modelcontextprotocol/server package, which prefers schemas already compliant with the Standard Schema spec [7][8][9].
Citations:
- 1: https://github.com/modelcontextprotocol/typescript-sdk/releases/tag/%40modelcontextprotocol/server%402.0.0-alpha.1
- 2: https://ts.sdk.modelcontextprotocol.io/v2/advanced/schema-libraries.html
- 3: https://github.com/modelcontextprotocol/typescript-sdk/blob/7d7e62cc/packages/core/src/util/standardSchema.ts
- 4: https://github.com/modelcontextprotocol/typescript-sdk/blob/7d7e62cc/.changeset/support-standard-json-schema.md
- 5: https://ts.sdk.modelcontextprotocol.io/v2/advanced/low-level-server.html
- 6: https://github.com/modelcontextprotocol/typescript-sdk/blob/ab552c30d5499b5e0aaa1ab47ee2481028910c3d/packages/core/src/util/standardSchema.ts
- 7: https://github.com/mastra-ai/mastra/blob/a7bbe773/packages/mcp/src/client/client.ts
- 8: https://github.com/mastra-ai/mastra/blob/ba9c14c7/packages/schema-compat/src/standard-schema/standard-schema.ts
- 9: https://app.unpkg.com/@mastra/schema-compat@1.3.4/files/dist/schema.d.ts
- 10: https://ts.sdk.modelcontextprotocol.io/v2/advanced/schema-libraries
- 11: https://github.com/modelcontextprotocol/typescript-sdk/blob/ab552c30d5499b5e0aaa1ab47ee2481028910c3d/packages/server/src/server/mcp.ts
- 12: https://modelcontextprotocol.io/specification/2026-07-28/server/tools
🌐 Web query:
typescript-sdk v2 standardSchemaToJsonSchema ~standard.jsonSchema definition
💡 Result:
In the Model Context Protocol (MCP) TypeScript SDK v2, standardSchemaToJsonSchema is an internal utility function located in @modelcontextprotocol/core that converts schemas compliant with the Standard Schema specification into JSON Schema format [1][2]. Key technical details include: 1. Function Purpose: It leverages the ~standard.jsonSchema property defined in the Standard Schema specification (specifically StandardJSONSchemaV1) to perform the conversion [1][3][4]. This allows the SDK to advertise tool and prompt arguments to MCP clients using the required JSON Schema format [1]. 2. Handling the ~standard Property: - Primary Path: If a schema provides a ~standard.jsonSchema implementation, the function invokes it to generate the JSON Schema [3][5]. - Fallback Path (Zod): For compatibility with older Zod versions that implement the Standard Schema interface but lack the JSON Schema extension (e.g., Zod 4.0–4.1), the SDK includes a fallback that uses the bundled z.toJSONSchema utility [3][6]. - Error Handling: If the provided schema does not implement the necessary JSON Schema interface and cannot be handled by the fallback, the function throws an error advising the user to upgrade their library or wrap their schema using fromJsonSchema [3][6]. 3. MCP Requirements: The function enforces specific constraints required by the MCP specification. Specifically, it ensures that the resulting JSON Schema has a root type of "object," as MCP tools and prompts require objects for their input/output definitions [1][7]. If a schema results in a different type (such as a string or number), it will throw an error, requiring the user to wrap their schema (e.g., using z.object({...})) [1][7]. 4. Public API Note: The MCP SDK documentation explicitly states that internal helpers like standardSchemaToJsonSchema are not part of the public API surface [2]. Developers are advised not to import them directly; instead, they should rely on the SDK's high-level abstractions for tool and prompt registration [2].
Citations:
- 1: https://github.com/modelcontextprotocol/typescript-sdk/blob/7d7e62cc/packages/core/src/util/standardSchema.ts
- 2: https://ts.sdk.modelcontextprotocol.io/v2/migration/upgrade-to-v2.html
- 3: https://github.com/modelcontextprotocol/typescript-sdk/blob/ab552c30d5499b5e0aaa1ab47ee2481028910c3d/packages/core/src/util/standardSchema.ts
- 4: https://github.com/modelcontextprotocol/ext-apps/blob/main/src/standard-schema.ts
- 5: https://github.com/modelcontextprotocol/typescript-sdk/blob/542d5c95860c03d0c1a689f579b925250e25de6c/packages/core/src/util/standardSchema.ts
- 6: fix(core): fall back to z.toJSONSchema for zod schemas without ~standard.jsonSchema modelcontextprotocol/typescript-sdk#1895
- 7: modelcontextprotocol/typescript-sdk@d6a02c8
🌐 Web query:
github modelcontextprotocol typescript-sdk standardSchemaToJsonSchema fallback z.toJSONSchema
💡 Result:
The standardSchemaToJsonSchema function within the Model Context Protocol (MCP) TypeScript SDK is designed to convert schemas—using the Standard Schema specification—into JSON Schema for use in MCP tools and prompts [1][2][3]. ### Fallback to z.toJSONSchema The function primarily relies on the ~standard.jsonSchema property of the provided schema [4][5]. However, it includes a fallback mechanism for Zod schemas that do not implement this interface [4][5][6]: * Zod 4.0–4.1: These versions implement StandardSchemaV1 but lack ~standard.jsonSchema [4][5][6]. The SDK falls back to the bundled z.toJSONSchema() with a one-time console warning, allowing these schemas to be converted without crashing the server [4][5][6]. * Zod 3 and non-Zod: If a schema (identified by its vendor) lacks ~standard.jsonSchema and is not a compatible Zod 4 instance, the function throws an explicit error [4][5][6]. For Zod 3 specifically, the error message directs users to upgrade to Zod >=4.2.0 or use fromJsonSchema() [4][5][6]. ### MCP Specification Requirements Beyond conversion, standardSchemaToJsonSchema ensures the output complies with MCP specifications [7][8]: * Root type: "object": The MCP protocol requires a top-level type: "object" for tool input/output schemas and prompt argument schemas [1][8]. Since Zod's discriminated unions often emit {oneOf: [...]} without a top-level type, standardSchemaToJsonSchema automatically enforces type: "object" when it is absent [1][7]. * Validation: If the schema has an explicitly non-object type (e.g., z.string()), the function throws an error because such types cannot satisfy the MCP specification for tool or prompt arguments [1][7][8]. The conversion target is hardcoded to 'draft-2020-12' to ensure consistency with the MCP specification [1][3][6]. Users are advised to upgrade to Zod >=4.2.0, as this version natively implements ~standard.jsonSchema, avoiding fallback behaviors and ensuring full compatibility [4][9].
Citations:
- 1: https://github.com/modelcontextprotocol/typescript-sdk/blob/7d7e62cc/packages/core/src/util/standardSchema.ts
- 2: https://github.com/modelcontextprotocol/typescript-sdk/blob/677511b5a7b68bb60bd7783ff3ff6e5dc9110eef/packages/core/src/util/standardSchema.ts
- 3: feat: support Standard Schema for tool/prompt schemas modelcontextprotocol/typescript-sdk#1689
- 4: https://github.com/modelcontextprotocol/typescript-sdk/blob/ab552c30d5499b5e0aaa1ab47ee2481028910c3d/packages/core/src/util/standardSchema.ts
- 5: https://github.com/modelcontextprotocol/typescript-sdk/blob/542d5c95860c03d0c1a689f579b925250e25de6c/packages/core/src/util/standardSchema.ts
- 6: fix(core): fall back to z.toJSONSchema for zod schemas without ~standard.jsonSchema modelcontextprotocol/typescript-sdk#1895
- 7: modelcontextprotocol/typescript-sdk@d6a02c8
- 8: fix(core): ensure standardSchemaToJsonSchema emits type:object modelcontextprotocol/typescript-sdk#1796
- 9: https://ts.sdk.modelcontextprotocol.io/v2/migration/upgrade-to-v2.html
Add coverage for the ~standard.jsonSchema conversion contract.
~standard.jsonSchema exists in Zod’s Standard Schema interface, but package upgrades can still change this internal behavior and move conversions to the MCP SDK fallback path. Add a short runtime test that exercises cacheJsonSchemaConversion() and assert the converter is found and cached, so future Zod/MCP changes do not silently disable the cache.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 44 - 68, Add a focused runtime test for
cacheJsonSchemaConversion that uses a Zod schema exposing ~standard.jsonSchema,
verifies the converter is present, and confirms repeated input conversions for
the same target are cached rather than recomputed. Keep the test scoped to the
existing conversion contract and ensure it would fail if the converter moved to
the MCP SDK fallback path.
| async function readRedashResource(uri: URL, variables: Variables) { | ||
| try { | ||
| const match = uri.match(/^redash:\/\/(query|dashboard)\/(\d+)$/); | ||
| const type = getResourceVariable(variables.type, "type", uri); | ||
| const resourceId = Number.parseInt(getResourceVariable(variables.id, "id", uri), 10); | ||
|
|
||
| if (!match) { | ||
| throw new Error(`Invalid resource URI: ${uri}`); | ||
| if (!Number.isSafeInteger(resourceId) || resourceId < 0) { | ||
| throw new Error(`Invalid resource URI: ${uri.href}`); | ||
| } | ||
|
|
||
| const [, type, id] = match; | ||
| const resourceId = parseInt(id, 10); | ||
|
|
||
| if (type === 'query') { | ||
| const query = await redashClient.getQuery(resourceId); | ||
| const result = await redashClient.executeQuery(resourceId); | ||
| if (type === "query") { | ||
| const [query, result] = await Promise.all([ | ||
| getRedashClient().getQuery(resourceId), | ||
| getRedashClient().executeQuery(resourceId), | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-numeric resource IDs.
Number.parseInt stops at the first non-digit character. redash://query/11abc parses to 11 and passes the Number.isSafeInteger guard. The server then reads query 11 for a URI that the guard was intended to reject.
Use a strict numeric check before parsing.
🐛 Proposed fix
const type = getResourceVariable(variables.type, "type", uri);
- const resourceId = Number.parseInt(getResourceVariable(variables.id, "id", uri), 10);
+ const rawId = getResourceVariable(variables.id, "id", uri);
+ if (!/^\d+$/.test(rawId)) {
+ throw new Error(`Invalid resource URI: ${uri.href}`);
+ }
+ const resourceId = Number.parseInt(rawId, 10);
if (!Number.isSafeInteger(resourceId) || resourceId < 0) {
throw new Error(`Invalid resource URI: ${uri.href}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function readRedashResource(uri: URL, variables: Variables) { | |
| try { | |
| const match = uri.match(/^redash:\/\/(query|dashboard)\/(\d+)$/); | |
| const type = getResourceVariable(variables.type, "type", uri); | |
| const resourceId = Number.parseInt(getResourceVariable(variables.id, "id", uri), 10); | |
| if (!match) { | |
| throw new Error(`Invalid resource URI: ${uri}`); | |
| if (!Number.isSafeInteger(resourceId) || resourceId < 0) { | |
| throw new Error(`Invalid resource URI: ${uri.href}`); | |
| } | |
| const [, type, id] = match; | |
| const resourceId = parseInt(id, 10); | |
| if (type === 'query') { | |
| const query = await redashClient.getQuery(resourceId); | |
| const result = await redashClient.executeQuery(resourceId); | |
| if (type === "query") { | |
| const [query, result] = await Promise.all([ | |
| getRedashClient().getQuery(resourceId), | |
| getRedashClient().executeQuery(resourceId), | |
| ]); | |
| async function readRedashResource(uri: URL, variables: Variables) { | |
| try { | |
| const type = getResourceVariable(variables.type, "type", uri); | |
| const rawId = getResourceVariable(variables.id, "id", uri); | |
| if (!/^\d+$/.test(rawId)) { | |
| throw new Error(`Invalid resource URI: ${uri.href}`); | |
| } | |
| const resourceId = Number.parseInt(rawId, 10); | |
| if (!Number.isSafeInteger(resourceId) || resourceId < 0) { | |
| throw new Error(`Invalid resource URI: ${uri.href}`); | |
| } | |
| if (type === "query") { | |
| const [query, result] = await Promise.all([ | |
| getRedashClient().getQuery(resourceId), | |
| getRedashClient().executeQuery(resourceId), | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 2163 - 2176, Update readRedashResource to validate
the extracted variables.id as an entirely numeric value before parsing,
rejecting values such as “11abc”; only parse and continue with the existing
safe-integer and non-negative checks after strict validation.
| function normalizeAttributes( | ||
| fields: LogFields | undefined, | ||
| seen: WeakSet<object> = new WeakSet(), | ||
| depth = 0, | ||
| ): LogAttributes | undefined { | ||
| if (!fields) { | ||
| return undefined; | ||
| } | ||
|
|
||
| seen.add(fields); | ||
| const attributes: LogAttributes = {}; | ||
| try { | ||
| for (const [key, value] of Object.entries(fields)) { | ||
| const normalized = normalizeAttributeValue(value, seen, depth); | ||
| if (normalized !== undefined) { | ||
| attributes[key] = normalized; | ||
| } | ||
| } | ||
| } catch { | ||
| return { normalization_error: UNSERIALIZABLE_ATTRIBUTE_VALUE }; | ||
| } | ||
| return attributes; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Shared object references are reported as [Circular].
normalizeAttributes adds fields to seen on line 131 and never removes it. For a nested object the entry is added twice: once on line 168 in normalizeAttributeValue, and once on line 131 when normalizeAttributes recurses. Only one removal happens, in the finally on line 179. The object therefore stays in seen after normalization completes.
The result is a false positive for shared, non-circular references. For { a: shared, b: shared } the value of b becomes "[Circular]" and the data is lost. True cycles are still detected correctly.
Remove the redundant add at the recursion site.
🐛 Proposed fix
function normalizeAttributes(
fields: LogFields | undefined,
seen: WeakSet<object> = new WeakSet(),
depth = 0,
): LogAttributes | undefined {
if (!fields) {
return undefined;
}
- seen.add(fields);
const attributes: LogAttributes = {};The root call from log no longer tracks the top-level fields object. Track it there instead, so a self-reference in the root is still caught:
- const attributes = normalizeAttributes(fields);
+ const seen = new WeakSet<object>();
+ if (fields) {
+ seen.add(fields);
+ }
+ const attributes = normalizeAttributes(fields, seen);Also applies to: 160-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/logger.ts` around lines 122 - 144, Remove the redundant seen.add(fields)
from normalizeAttributes so nested objects are tracked only by
normalizeAttributeValue and removed correctly after processing. Preserve cycle
detection by ensuring the root fields object is added to the seen set at the log
entry point before normalization, so self-references remain reported as circular
while shared references normalize normally.
| const requestFields: LogFields = { | ||
| "http.request.method": "POST", | ||
| "url.path": path, | ||
| "redash.data_source.id": queryData.data_source_id, | ||
| "redash.request.header.names": Object.keys(this.client.defaults.headers || {}), | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
redash.request.header.names emits Axios method buckets in both query methods. Both sites call Object.keys(this.client.defaults.headers || {}). Axios stores defaults as a HeadersDefaults object whose top-level keys are common, delete, get, head, post, put, and patch. The headers passed to axios.create({ headers }) land in the common bucket. The logged field therefore never contains a real header name.
src/redashClient.ts#L440-L445: increateQuery, readthis.client.defaults.headers?.commonbefore callingObject.keys.src/redashClient.ts#L492-L500: inupdateQuery, apply the same change so both log fields report real header names.
📍 Affects 1 file
src/redashClient.ts#L440-L445(this comment)src/redashClient.ts#L492-L500
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/redashClient.ts` around lines 440 - 445, Update the
redash.request.header.names field in createQuery at src/redashClient.ts:440-445
and updateQuery at src/redashClient.ts:492-500 to read
this.client.defaults.headers?.common before calling Object.keys, preserving the
existing empty fallback so both methods log actual header names instead of Axios
method buckets.
| function parseDiagLogLevel(value: string | undefined): DiagLogLevel { | ||
| const normalized = value?.trim().toUpperCase(); | ||
| return normalized && normalized in DiagLogLevel | ||
| ? DiagLogLevel[normalized as keyof typeof DiagLogLevel] | ||
| : DiagLogLevel.WARN; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Numeric OTEL_LOG_LEVEL values resolve to a string level.
DiagLogLevel is a numeric enum, so it also contains reverse-mapped keys such as "30". If OTEL_LOG_LEVEL is set to a number, normalized in DiagLogLevel is true and DiagLogLevel[normalized] returns the level name instead of a number. The API then receives an invalid log level. Restrict the lookup to non-numeric keys.
🛠️ Proposed fix
function parseDiagLogLevel(value: string | undefined): DiagLogLevel {
const normalized = value?.trim().toUpperCase();
- return normalized && normalized in DiagLogLevel
+ return normalized
+ && normalized in DiagLogLevel
+ && typeof DiagLogLevel[normalized as keyof typeof DiagLogLevel] === "number"
? DiagLogLevel[normalized as keyof typeof DiagLogLevel]
: DiagLogLevel.WARN;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseDiagLogLevel(value: string | undefined): DiagLogLevel { | |
| const normalized = value?.trim().toUpperCase(); | |
| return normalized && normalized in DiagLogLevel | |
| ? DiagLogLevel[normalized as keyof typeof DiagLogLevel] | |
| : DiagLogLevel.WARN; | |
| } | |
| function parseDiagLogLevel(value: string | undefined): DiagLogLevel { | |
| const normalized = value?.trim().toUpperCase(); | |
| return normalized | |
| && normalized in DiagLogLevel | |
| && typeof DiagLogLevel[normalized as keyof typeof DiagLogLevel] === "number" | |
| ? DiagLogLevel[normalized as keyof typeof DiagLogLevel] | |
| : DiagLogLevel.WARN; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/telemetry.ts` around lines 398 - 403, Update parseDiagLogLevel to reject
numeric normalized values before checking DiagLogLevel membership, ensuring only
non-numeric enum keys are looked up and numeric OTEL_LOG_LEVEL inputs fall back
to DiagLogLevel.WARN.
afd20f0 to
e0e3ad6
Compare
There was a problem hiding this comment.
Pull request overview
This PR expands the Redash MCP server with safer BigQuery schema discovery, adds a production-ready Streamable HTTP transport, and introduces OpenTelemetry-based observability and improved packaging/release ergonomics.
Changes:
- Implement paginated BigQuery schema discovery tools (datasets, tables, columns) to avoid unbounded Redash schema loads.
- Add Streamable HTTP server mode with Host/Origin validation, CORS handling, and graceful shutdown orchestration.
- Add OpenTelemetry tracing/metrics/logs plumbing + tests, and modernize packaging (typed exports), workflows, and Docker/pnpm tooling.
Reviewed changes
Copilot reviewed 41 out of 48 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Enable declaration emit for published TypeScript types. |
| test/otel-export.test.mjs | Add OTLP/HTTP integration test for correlated traces/metrics/logs. |
| test/fixtures/otel-app.mjs | Fixture app that boots telemetry + HTTP server for OTel export testing. |
| src/utils.ts | Add shared formatError helper. |
| src/telemetryConfig.ts | Parse/resolve telemetry env config for OTLP + Prometheus. |
| src/telemetry.ts | Implement OpenTelemetry SDK initialization, exporters, Prometheus embedding, shutdown. |
| src/startup.ts | Centralize CLI startup flow + graceful shutdown hooks. |
| src/redashClient.ts | Add safer structured logging controls + data source detail API + lazy singleton getter. |
| src/packageInfo.ts | Derive runtime package version robustly for server metadata. |
| src/opentelemetry-semconv-incubating.d.ts | Provide TS declaration shim for incubating semantic conventions in tests. |
| src/mcpTelemetry.ts | Instrument MCP transport boundary with spans/metrics and optional content capture. |
| src/mcpProtocol.ts | Export a pinned “modern” MCP protocol revision constant for tests/clients. |
| src/logger.ts | Emit logs to stderr + OTel Logs, with structured fields normalization and MCP client notifications. |
| src/jsonSchema.ts | Remove custom Zod→JSON Schema converter (superseded by current approach). |
| src/httpServer.ts | Add Streamable HTTP transport server with allowlists, CORS, and shutdown handling. |
| src/config.ts | Add CLI/env parsing for stdio vs HTTP transport and allowlist enforcement. |
| src/cli.ts | Switch CLI entrypoint to defer imports until after env load and telemetry init. |
| src/chartVisualization.ts | Improve schema descriptions and tighten record typing for chart updates. |
| src/bigQuerySchema.ts | Implement bounded/paginated BigQuery INFORMATION_SCHEMA discovery through Redash. |
| src/tests/telemetryConfig.test.ts | Unit tests for telemetry config resolution logic. |
| src/tests/startup.test.ts | Tests for graceful shutdown controller behavior. |
| src/tests/setupEnv.ts | Jest setup file for injecting Redash env vars early. |
| src/tests/redashClient.test.ts | Expand client tests for safe logging/content capture behavior and new APIs. |
| src/tests/mcpTelemetry.test.ts | Unit tests for MCP span/attribute behavior and error classification. |
| src/tests/mcpServer.test.ts | Validate tool registration, version metadata, and protocol matrix behaviors. |
| src/tests/logger.test.ts | Tests for stderr logging, OTel log export, normalization, and client notification failure handling. |
| src/tests/jsonSchema.test.ts | Remove tests tied to deleted JSON schema converter. |
| src/tests/integration.test.ts | Update integration tests to use tool definitions and lazy client creation. |
| src/tests/httpServer.test.ts | Add coverage for HTTP transport behavior, allowlists/CORS, shutdown, and protocol matrix. |
| src/tests/config.test.ts | Add coverage for CLI/env config parsing and allowlist validation. |
| src/tests/clientFixtures.ts | Shared protocol negotiation fixtures for legacy/modern client test matrix. |
| src/tests/bigQuerySchema.test.ts | Unit tests for bounded BigQuery schema discovery behavior and safety validations. |
| README.md | Document HTTP mode, telemetry, safe content capture, BigQuery schema pagination, and pnpm usage. |
| playwright.config.ts | Switch inspector runner command from npm to pnpm. |
| package.json | Add typed exports, pnpm standardization, OTel deps, new scripts (incl. OTel integration test). |
| jest.config.js | Add setupFiles entry to preload test env vars. |
| e2e/inspector-basic.spec.ts | Update E2E assertion to avoid brittle “tool count” expectation. |
| Dockerfile | Add multi-stage pnpm build + runtime defaults for HTTP mode in container. |
| .npmrc | Default to ignoring lifecycle scripts during installs. |
| .gitignore | Ignore package-lock.json. |
| .github/workflows/test.yml | Switch CI to pnpm + add OTel export integration test step. |
| .github/workflows/tagpr.yml | Switch tagpr workflow install step to pnpm. |
| .github/workflows/release.yml | Add release ref validation, GHCR publish/signing flow, pnpm-based build/test, conditional npm publish. |
| .env.example | Document new transport + telemetry environment variables. |
| .dockerignore | Exclude dev/test artifacts and sources from Docker build context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const response = await this.client.get(`/api/data_sources/${dataSourceId}`); | ||
| return response.data; | ||
| } catch (error) { | ||
| logger.error(`Error fetching data source ${dataSourceId}: ${error}`); |
| diag.setLogger(new StderrDiagLogger(), { | ||
| logLevel: parseDiagLogLevel(configuredLogLevel), | ||
| suppressOverrideMessage: true, | ||
| }); | ||
| sdk.start(); | ||
|
|
||
| if (metricReaders.length > 0) { | ||
| new HostMetrics({ name: SERVICE_NAME }).start(); | ||
| } |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/redashClient.ts (2)
553-563: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse structured logging in the new
getDataSourcemethod.Line 560 interpolates
errorinto the message string. The new methods in this change pass fields and the error object tologger.errorinstead. Match that pattern so the data source id and the error are queryable.♻️ Proposed refactor
- logger.error(`Error fetching data source ${dataSourceId}: ${error}`); + logger.error(`Error fetching data source ${dataSourceId}`, { + "redash.data_source.id": dataSourceId, + }, error);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/redashClient.ts` around lines 553 - 563, Update the error handling in getDataSource to use structured logger.error fields instead of interpolating error into the message; pass dataSourceId and the caught error as queryable properties while preserving the existing failure message and rethrow behavior.
455-474: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
axios.isAxiosErrorfor consistent error narrowing.
executeQuery,pollQueryResults, andgetQueryResultsAsCsvnarrow errors withaxios.isAxiosError.createQueryandupdateQueryuse a manual structural cast instead. The manual cast treats any object with aresponseproperty as an Axios error. Align both methods with the rest of the file.♻️ Proposed refactor for
createQuery- const axiosError = error as { - message?: string; - request?: unknown; - response?: { status?: unknown }; - }; - if (axiosError.response) { - throw new Error(`Failed to create query: Redash API error (${axiosError.response.status})`); - } - if (axiosError.request) { - throw new Error(`Failed to create query: No response received from Redash API: ${axiosError.message}`); - } + if (axios.isAxiosError(error)) { + if (error.response) { + throw new Error(`Failed to create query: Redash API error (${error.response.status})`); + } + if (error.request) { + throw new Error(`Failed to create query: No response received from Redash API: ${error.message}`); + } + } throw new Error(`Failed to create query: ${error instanceof Error ? error.message : String(error)}`);Apply the same change in
updateQueryat Lines 514-524.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/redashClient.ts` around lines 455 - 474, Update the error handling in createQuery and updateQuery to use axios.isAxiosError(error) instead of manual structural casts. Preserve the existing response, request, and fallback error-message branches while narrowing only confirmed Axios errors consistently with executeQuery, pollQueryResults, and getQueryResultsAsCsv.src/index.ts (1)
912-968: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured
logger.errorfields for the new BigQuery tool handlers.
listBigQueryDatasets,listBigQueryTables, andgetBigQueryTableSchemacalllogger.errorwith a single interpolated string (for example, Line 920:`Error listing BigQuery datasets for data source ${params.dataSourceId}: ${error}`). Every other handler updated in this diff (Lines 428, 552, 584, 684, 761, 797, 863) passes structured fields and the rawerrorobject as separate arguments tologger.error(message, fields, error). Interpolatingerrorinto the message string drops the stack trace and skips the structured fields that the rest of the file relies on for correlation.Align these three handlers with the established pattern.
♻️ Proposed fix for the three new handlers
} catch (error) { - logger.error(`Error listing BigQuery datasets for data source ${params.dataSourceId}: ${error}`); + logger.error( + `Error listing BigQuery datasets for data source ${params.dataSourceId}`, + { "redash.data_source.id": params.dataSourceId }, + error, + ); return {} catch (error) { - logger.error(`Error listing BigQuery tables for ${params.dataset}: ${error}`); + logger.error( + `Error listing BigQuery tables for ${params.dataset}`, + { "redash.data_source.id": params.dataSourceId, "bigquery.dataset": params.dataset }, + error, + ); return {} catch (error) { - logger.error(`Error getting BigQuery table schema for ${params.dataset}.${params.table}: ${error}`); + logger.error( + `Error getting BigQuery table schema for ${params.dataset}.${params.table}`, + { "redash.data_source.id": params.dataSourceId, "bigquery.dataset": params.dataset, "bigquery.table": params.table }, + error, + ); return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 912 - 968, Update the catch blocks in listBigQueryDatasets, listBigQueryTables, and getBigQueryTableSchema to call logger.error with the descriptive message, relevant structured request fields, and the raw error object as separate arguments, matching the established handler pattern; do not interpolate error into the message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/index.ts`:
- Around line 912-968: Update the catch blocks in listBigQueryDatasets,
listBigQueryTables, and getBigQueryTableSchema to call logger.error with the
descriptive message, relevant structured request fields, and the raw error
object as separate arguments, matching the established handler pattern; do not
interpolate error into the message.
In `@src/redashClient.ts`:
- Around line 553-563: Update the error handling in getDataSource to use
structured logger.error fields instead of interpolating error into the message;
pass dataSourceId and the caught error as queryable properties while preserving
the existing failure message and rethrow behavior.
- Around line 455-474: Update the error handling in createQuery and updateQuery
to use axios.isAxiosError(error) instead of manual structural casts. Preserve
the existing response, request, and fallback error-message branches while
narrowing only confirmed Axios errors consistently with executeQuery,
pollQueryResults, and getQueryResultsAsCsv.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b21359c-d8ef-48d4-beae-93312b86d4f2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.env.example.github/workflows/release.yml.github/workflows/test.ymlREADME.mdpackage.jsonsrc/__tests__/bigQuerySchema.test.tssrc/__tests__/httpServer.test.tssrc/__tests__/logger.test.tssrc/__tests__/mcpServer.test.tssrc/__tests__/mcpTelemetry.test.tssrc/__tests__/redashClient.test.tssrc/__tests__/telemetryConfig.test.tssrc/bigQuerySchema.tssrc/cli.tssrc/httpServer.tssrc/index.tssrc/logger.tssrc/mcpTelemetry.tssrc/opentelemetry-semconv-incubating.d.tssrc/packageInfo.tssrc/redashClient.tssrc/startup.tssrc/telemetry.tssrc/telemetryConfig.tstest/fixtures/otel-app.mjstest/otel-export.test.mjstsconfig.json
💤 Files with no reviewable changes (1)
- .github/workflows/release.yml
🚧 Files skipped from review as they are similar to previous changes (23)
- tsconfig.json
- .github/workflows/test.yml
- .env.example
- test/fixtures/otel-app.mjs
- src/tests/redashClient.test.ts
- src/cli.ts
- src/tests/telemetryConfig.test.ts
- src/tests/httpServer.test.ts
- src/packageInfo.ts
- src/httpServer.ts
- src/startup.ts
- src/tests/logger.test.ts
- test/otel-export.test.mjs
- src/tests/mcpServer.test.ts
- README.md
- package.json
- src/opentelemetry-semconv-incubating.d.ts
- src/tests/bigQuerySchema.test.ts
- src/bigQuerySchema.ts
- src/mcpTelemetry.ts
- src/telemetryConfig.ts
- src/telemetry.ts
- src/logger.ts
e0e3ad6 to
493e35f
Compare
655f7e3 to
f87fb41
Compare
493e35f to
ed77938
Compare
f87fb41 to
9daefeb
Compare
ed77938 to
c883a24
Compare
- Default `time` and `until` to `null` (not just `day_of_week`), since Redash's scheduler directly indexes all three keys - Restrict `day_of_week` to valid English weekday names via z.enum, preventing invalid strings that would fail calendar.day_name lookup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9daefeb to
d59373b
Compare
…ek-default fix: add type-safe schedule schema with day_of_week default
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
69dd484 to
e06d98f
Compare
Summary by CodeRabbit