Merged
Conversation
- Remove token.is_empty() check that was preventing HTTP header auth - Always check HTTP Authorization header when admin credential is configured - Add mcporter integration test to verify third-party MCP clients work - Test exercises admin credential auth, JWT auth, and failure cases
Contributor
|
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:
📝 WalkthroughWalkthroughCentralizes MCP tool authentication into a new async Changes
Sequence Diagram(s)sequenceDiagram
participant Client as mcporter / HTTP client
participant Handler as Ad4mMcpHandler
participant Session as SessionStore
participant JWT as JWT/Capability Validator
Client->>Handler: call_tool(request + optional Authorization header)
Handler->>Session: read session token (if any)
alt Authorization header present
Handler->>JWT: verify Bearer JWT OR constant-time compare to admin_credential
JWT-->>Handler: verified + capabilities
Handler->>Session: store token in session (if valid)
else Session token present
Handler->>JWT: validate session JWT OR compare to admin_credential
JWT-->>Handler: verified + capabilities
else No tokens & no admin_credential
Handler-->>Client: allow (single-user localhost trust)
end
Handler-->>Client: allow / reject ("Authentication required")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-executor/src/mcp/tools/mod.rs`:
- Around line 173-180: check_auth() currently rejects any valid JWT lacking
PERSPECTIVE_CREATE_CAPABILITY; change it to only validate the token/JWT. In the
block using capabilities_from_token(session_token.clone(),
admin_cred.map(String::from)) (and the analogous HTTP-header JWT branch using
capabilities_from_token for header tokens), return true when the token parses Ok
and the capabilities object is non-empty or otherwise indicates a valid
principal, but remove the check_capability(&caps,
&PERSPECTIVE_CREATE_CAPABILITY) gating; leave capability checks
(check_capability with PERSPECTIVE_CREATE_CAPABILITY) to the individual handler
code instead.
- Around line 186-191: The Authorization parsing currently uses
strip_prefix("Bearer ") which is case-sensitive; update the HttpRequestParts
extraction (the http_header_value variable) to handle the "Bearer " scheme
case-insensitively: after obtaining the header string h from
HttpRequestParts.headers, check h for a case-insensitive "Bearer " prefix (e.g.,
compare the first 7 chars with eq_ignore_ascii_case or lowercasing only that
prefix) and if present slice off those 7 bytes to get the token, otherwise use h
as-is; keep this logic inside the same chain that builds http_header_value so
callers of http_header_value continue to receive the raw token string.
In `@tests/js/tests/mcp-mcporter.test.ts`:
- Around line 49-69: Before calling startExecutor in the before() hook, detect
and terminate any lingering ad4m-executor processes to avoid port conflicts: add
logic in the before hook (around where startExecutor(...) is invoked) to find
running processes named "ad4m-executor" (or processes listening on the fixed
ports) and kill them (use cross-platform commands: pgrep/kill on Unix and
tasklist/taskkill on Windows), wait for termination to complete, then proceed to
create appDataPath and call startExecutor; ensure this cleanup runs before
invoking startExecutor and that the code awaits the kill operation to guarantee
ports are free.
- Around line 11-12: The test file assigns fetch to global (global.fetch =
fetch) but never imports it, causing a module-load failure; import a compatible
fetch implementation (e.g., node-fetch or undici's fetch) at the top of the file
and then assign it to global.fetch (keeping or removing the existing
//@ts-ignore as needed for TypeScript), so update the file to import fetch and
then perform the global.fetch = fetch assignment; locate the assignment
expression "global.fetch = fetch" to apply this change.
- Around line 115-118: Replace the unsafe execSync shell-string calls with
execFileSync using argument arrays: locate each use of execSync in this test
(calls that build commands like `mcporter list ad4m --config
${mcporterConfigPath}` and the other nine sites) and change them to call
execFileSync('mcporter', ['list','ad4m','--config', mcporterConfigPath], {
encoding: 'utf-8', timeout: 10000 }) (preserve the same options), and for calls
that pass JSON or dynamic IDs (the site embedding request IDs/codes into JSON)
pass those values as separate args or write the JSON to a temp file and pass its
path so you avoid shell interpolation; ensure you import execFileSync from
child_process and update all 10 call sites consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d572269-d105-4629-995b-c746bccd7d16
📒 Files selected for processing (4)
rust-executor/src/mcp/tools/mod.rstests/js/package.jsontests/js/tests/mcp-auth.test.tstests/js/tests/mcp-mcporter.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.circleci/config.yml (2)
73-76: Usecorepack enableto enforce the pinned pnpm version instead of unpinned global installation.The repo already declares
pnpm@9.15.0inpackage.jsonvia"packageManager". Usingnpm install -g pnpmbypasses this pin and allows the installed version to drift based on the CI environment. Replace this withcorepack enableto activate the pinned version enforced by Node's corepack (available in Node 16.13+).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.circleci/config.yml around lines 73 - 76, In the "Install pnpm (if needed)" command block, remove the unpinned global install (the command that checks for pnpm and runs npm install -g pnpm) and instead run corepack to enable the repo-pinned package manager; specifically, call corepack enable (and optionally corepack prepare pnpm@<pinned-version> --activate) before checking/printing pnpm version so the pinned pnpm from package.json's packageManager is used; update the lines that reference the existing check and echo ("command -v pnpm >/dev/null 2>&1 || npm install -g pnpm" and 'echo "pnpm: $(pnpm --version)"') to use corepack enable and then print pnpm --version.
233-234: Addrequiresif this workflow should wait for smoke tests before building.In CircleCI, job order in the YAML list does not enforce execution order—jobs with no dependencies run concurrently by default. To make
build-and-testwait fornode24-smoke-test, add an explicit dependency:Suggested YAML change
- node24-smoke-test - - build-and-test + - build-and-test: + requires: + - node24-smoke-test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.circleci/config.yml around lines 233 - 234, The workflow currently lists node24-smoke-test before build-and-test but has no dependency, so jobs may run concurrently; update the workflow entry for build-and-test to include a requires dependency on node24-smoke-test (i.e., add requires: [node24-smoke-test] to the build-and-test job definition) so build-and-test waits for node24-smoke-test to complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.circleci/config.yml:
- Around line 84-85: The CI job invoking "pnpm run build-npm-packages" is
failing on Node 24 because the underlying `@coasys/ad4m` build (script buildSchema
referenced in core/package.json) uses the deprecated
--es-module-specifier-resolution flag; update the buildSchema script (in
core/package.json) to remove that flag and replace it with a Node-24-compatible
solution (for example, adjust the bundler/tsconfig/moduleResolution or use a
compatible bundler plugin/transform) so the `@coasys/ad4m`#build completes without
the deprecated flag and the CI smoke test on Node 24 passes.
---
Nitpick comments:
In @.circleci/config.yml:
- Around line 73-76: In the "Install pnpm (if needed)" command block, remove the
unpinned global install (the command that checks for pnpm and runs npm install
-g pnpm) and instead run corepack to enable the repo-pinned package manager;
specifically, call corepack enable (and optionally corepack prepare
pnpm@<pinned-version> --activate) before checking/printing pnpm version so the
pinned pnpm from package.json's packageManager is used; update the lines that
reference the existing check and echo ("command -v pnpm >/dev/null 2>&1 || npm
install -g pnpm" and 'echo "pnpm: $(pnpm --version)"') to use corepack enable
and then print pnpm --version.
- Around line 233-234: The workflow currently lists node24-smoke-test before
build-and-test but has no dependency, so jobs may run concurrently; update the
workflow entry for build-and-test to include a requires dependency on
node24-smoke-test (i.e., add requires: [node24-smoke-test] to the build-and-test
job definition) so build-and-test waits for node24-smoke-test to complete.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.github/workflows/publish_staging.yml (4)
548-552: Updateactions/setup-nodeto v4.Same issue as other jobs -
actions/setup-node@v3should be updated to v4.♻️ Proposed fix
- name: Use Node.js 24.x - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: 24.x registry-url: 'https://registry.npmjs.org'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish_staging.yml around lines 548 - 552, The workflow step labeled "Use Node.js 24.x" currently references actions/setup-node@v3; update that step to use actions/setup-node@v4 (i.e., change the uses value from actions/setup-node@v3 to actions/setup-node@v4) while keeping the existing inputs (node-version: 24.x and registry-url) unchanged to bring this job in line with other updated jobs.
459-463: Updateactions/setup-nodeto v4.Same issue as the macOS build -
actions/setup-node@v3should be updated to v4.♻️ Proposed fix
- name: Use Node.js 24.x - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: 24.x cache: 'pnpm'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish_staging.yml around lines 459 - 463, Replace the outdated action reference actions/setup-node@v3 with actions/setup-node@v4 in the GitHub Actions step titled "Use Node.js 24.x" (the block containing the uses: actions/setup-node@v3 and with: node-version: 24.x, cache: 'pnpm'), ensuring the step now references actions/setup-node@v4; verify that the existing with inputs (node-version and cache) remain valid for v4 and update them if needed to the v4-compatible input names.
279-283: Updateactions/setup-nodeto v4.The static analysis tool reports that
actions/setup-node@v3is outdated. Consider updating toactions/setup-node@v4for better compatibility and continued support.♻️ Proposed fix
- name: Use Node.js 24.x - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: 24.x cache: 'pnpm'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish_staging.yml around lines 279 - 283, Replace the outdated GitHub Action usage string "actions/setup-node@v3" with "actions/setup-node@v4" in the workflow step that currently reads uses: actions/setup-node@v3 (the step configuring node-version: 24.x and cache: 'pnpm'), keep the existing with keys (node-version and cache) intact, and then verify the workflow runs successfully in CI since v4 may require no other changes but should be validated.
16-19: Consider updatingactions/setup-nodeto v4 here as well.For consistency with the recommended updates in other jobs, this instance should also use v4.
♻️ Proposed fix
- name: setup node - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: 24🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish_staging.yml around lines 16 - 19, The workflow uses actions/setup-node@v3 in the "setup node" step; update that to actions/setup-node@v4 for consistency with other jobs. Locate the step with name: setup node (using actions/setup-node@v3 and node-version: 24) and change the GitHub Action reference to actions/setup-node@v4 while keeping the node-version and other fields unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/publish_staging.yml:
- Around line 548-552: The workflow step labeled "Use Node.js 24.x" currently
references actions/setup-node@v3; update that step to use actions/setup-node@v4
(i.e., change the uses value from actions/setup-node@v3 to
actions/setup-node@v4) while keeping the existing inputs (node-version: 24.x and
registry-url) unchanged to bring this job in line with other updated jobs.
- Around line 459-463: Replace the outdated action reference
actions/setup-node@v3 with actions/setup-node@v4 in the GitHub Actions step
titled "Use Node.js 24.x" (the block containing the uses: actions/setup-node@v3
and with: node-version: 24.x, cache: 'pnpm'), ensuring the step now references
actions/setup-node@v4; verify that the existing with inputs (node-version and
cache) remain valid for v4 and update them if needed to the v4-compatible input
names.
- Around line 279-283: Replace the outdated GitHub Action usage string
"actions/setup-node@v3" with "actions/setup-node@v4" in the workflow step that
currently reads uses: actions/setup-node@v3 (the step configuring node-version:
24.x and cache: 'pnpm'), keep the existing with keys (node-version and cache)
intact, and then verify the workflow runs successfully in CI since v4 may
require no other changes but should be validated.
- Around line 16-19: The workflow uses actions/setup-node@v3 in the "setup node"
step; update that to actions/setup-node@v4 for consistency with other jobs.
Locate the step with name: setup node (using actions/setup-node@v3 and
node-version: 24) and change the GitHub Action reference to
actions/setup-node@v4 while keeping the node-version and other fields unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29e7b30d-ee43-4a7d-b701-e4835f2b97a3
📒 Files selected for processing (3)
.circleci/Dockerfile.github/workflows/publish.yml.github/workflows/publish_staging.yml
Updates all CI/CD references to use the new Node 24 image: - Digest: sha256:3605cfd2c0aee389ac9e7bb499a8e2633d0367ef3cc148d6668b2dbdff846ce0 - Image size: ~23GB with Node 24, CUDA 12.5, Rust 1.92, Holochain 0.7.0-dev.10
1. rust-executor/src/mcp/tools/mod.rs: - Remove PERSPECTIVE_CREATE_CAPABILITY check from auth (just validate token) - Parse Authorization header case-insensitively (handle 'bearer' vs 'Bearer') 2. tests/js/tests/mcp-mcporter.test.ts: - Import fetch from node-fetch before global assignment - Kill lingering executors before tests to avoid port conflicts - Replace all execSync with execFileSync for security 3. core/package.json: - Remove deprecated --es-module-specifier-resolution flag (Node 24 compatible)
lucksus
commented
Mar 8, 2026
lucksus
commented
Mar 8, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish.yml (1)
128-131: 🛠️ Refactor suggestion | 🟠 MajorOutdated
actions/setup-node@v1should be upgraded.This step uses
actions/setup-node@v1while the rest of the file uses@v3. Version 1 is significantly outdated and may have compatibility issues with Node 24.x. Consider upgrading to v3 or v4 for consistency.♻️ Proposed fix
- name: Use Node.js ${{ matrix.node-version }} - uses: actions/setup-node@v1 + uses: actions/setup-node@v4 with: node-version: ${{ matrix.node-version }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml around lines 128 - 131, Replace the outdated GitHub Action usage "uses: actions/setup-node@v1" with a current major version (e.g., "actions/setup-node@v3" or "@v4") to match the rest of the workflow and ensure compatibility with Node 24.x; update the step that declares uses: actions/setup-node@v1 so it points to the chosen newer version while keeping the existing with: node-version: ${{ matrix.node-version }} configuration.
🧹 Nitpick comments (2)
.github/workflows/publish_staging.yml (1)
279-283: Consider upgradingactions/setup-nodeto v4.Static analysis flagged
actions/setup-node@v3as outdated. While v3 still works, v4 offers better Node.js version support and is recommended for newer Node versions like 24.x. The same applies to the other occurrences in this file (lines 460, 549).♻️ Suggested upgrade
- - name: Use Node.js 24.x - uses: actions/setup-node@v3 + - name: Use Node.js 24.x + uses: actions/setup-node@v4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish_staging.yml around lines 279 - 283, Replace the outdated GitHub Action usage of actions/setup-node@v3 with actions/setup-node@v4 wherever it's used (e.g., the workflow step named "Use Node.js 24.x" that currently has uses: actions/setup-node@v3 and other similar steps in this file); update the `uses:` value to `actions/setup-node@v4` while keeping the existing `with:` inputs (node-version: 24.x, cache: 'pnpm') intact so behavior remains the same but benefits from v4 improvements..circleci/config.yml (1)
115-122: Port cleanup list expanded appropriately.The comprehensive port cleanup helps prevent conflicts on the self-hosted runner. However, the hardcoded port list could become stale if test configurations change.
Consider extracting the port list to a shared location or using a more dynamic approach (e.g., killing all processes owned by the test user on a port range) to reduce maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.circleci/config.yml around lines 115 - 122, The hardcoded port cleanup loop in .circleci/config.yml (the for port in ...; do lsof -ti:$port | xargs -r kill -9 ...) can get stale; extract the port list into a single reusable variable or parameter (e.g., CIRCLE_TEST_PORTS) or implement a dynamic range-based cleanup (e.g., iterate over a port range or use lsof to find all listening ports owned by the test user) and replace the inline list in the for loop so the ports are maintained in one place and the cleanup adapts to config changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/package.json`:
- Line 10: The buildSchema script in package.json currently runs "node
lib/src/buildSchema.js" which fails at runtime due to extension-less imports
emitted by tsc; fix by either (A) updating all TypeScript import statements
referenced by lib/src/buildSchema.js (and its imported modules like
AgentResolver) to include ".js" extensions so the Node ESM loader can resolve
them, or (B) change the script to run the transpiled file with a loader that
tolerates extension-less imports (e.g., "tsx lib/src/buildSchema.js"), or (C)
restore the Node flag by replacing the script with "node
--es-module-specifier-resolution=node lib/src/buildSchema.js" and ensure
package.json/README notes the minimum Node version required for that flag; pick
one approach and apply consistently to the buildSchema invocation and its module
imports.
---
Outside diff comments:
In @.github/workflows/publish.yml:
- Around line 128-131: Replace the outdated GitHub Action usage "uses:
actions/setup-node@v1" with a current major version (e.g.,
"actions/setup-node@v3" or "@v4") to match the rest of the workflow and ensure
compatibility with Node 24.x; update the step that declares uses:
actions/setup-node@v1 so it points to the chosen newer version while keeping the
existing with: node-version: ${{ matrix.node-version }} configuration.
---
Nitpick comments:
In @.circleci/config.yml:
- Around line 115-122: The hardcoded port cleanup loop in .circleci/config.yml
(the for port in ...; do lsof -ti:$port | xargs -r kill -9 ...) can get stale;
extract the port list into a single reusable variable or parameter (e.g.,
CIRCLE_TEST_PORTS) or implement a dynamic range-based cleanup (e.g., iterate
over a port range or use lsof to find all listening ports owned by the test
user) and replace the inline list in the for loop so the ports are maintained in
one place and the cleanup adapts to config changes.
In @.github/workflows/publish_staging.yml:
- Around line 279-283: Replace the outdated GitHub Action usage of
actions/setup-node@v3 with actions/setup-node@v4 wherever it's used (e.g., the
workflow step named "Use Node.js 24.x" that currently has uses:
actions/setup-node@v3 and other similar steps in this file); update the `uses:`
value to `actions/setup-node@v4` while keeping the existing `with:` inputs
(node-version: 24.x, cache: 'pnpm') intact so behavior remains the same but
benefits from v4 improvements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43279813-bcb1-4bd3-90b3-6516e43585b5
📒 Files selected for processing (10)
.circleci/config.yml.github/workflows/agent-language-tests.yml.github/workflows/direct-message-language-tests.yml.github/workflows/file-storage-language-tests.yml.github/workflows/p-diff-sync-tests.yml.github/workflows/publish.yml.github/workflows/publish_staging.ymlcore/package.jsonrust-executor/src/mcp/tools/mod.rstests/js/tests/mcp-mcporter.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
core/fix-imports.mjs (2)
18-28: Redundant negative lookahead in regex.The
(?!\.js)lookahead at line 18 doesn't filter anything because it checks the position right before the closing quote (which will never be.js). The actual extension filtering is done by the secondary check at line 22 with/\.\w+$/.The code works correctly due to the secondary check, but the regex could be simplified:
- const importRegex = /from\s+["'](\.\.?\/[^"']+?)(?!\.js)["']/g; + const importRegex = /from\s+["'](\.\.?\/[^"']+?)["']/g;Same applies to
dynamicImportRegexat line 32.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/fix-imports.mjs` around lines 18 - 28, The regexes importRegex and dynamicImportRegex contain an unnecessary negative lookahead (?!\.js) which never matches in that position and is redundant because the code already filters extensions with the secondary check importPath.match(/\.\w+$/). Remove the (?!\.js) from both regexes (i.e., simplify /from\s+["'](\.\.?\/[^"']+?)(?!\.js)["']/g and the analogous dynamicImportRegex) so they just capture the import path, keeping the existing callback logic that checks importPath.match(/\.\w+$/) and appends '.js' when needed.
63-64: Add error handling for missing directory.If
srcDirdoesn't exist (e.g., iftscfailed silently), the script will throw an unclearENOENTerror fromreaddirSync. Consider adding a guard:🛡️ Proposed fix
+import { existsSync } from 'fs'; + const srcDir = process.argv[2] || './lib/src'; + +if (!existsSync(srcDir)) { + console.error(`Error: Directory not found: ${srcDir}`); + console.error('Make sure tsc has completed successfully.'); + process.exit(1); +} console.log(`Processing JS files in ${srcDir}...`); walkDir(srcDir);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/fix-imports.mjs` around lines 63 - 64, The script currently prints "Processing JS files in ${srcDir}..." and calls walkDir(srcDir) without checking that srcDir exists, which leads to an opaque ENOENT from readdirSync; before logging and calling walkDir, guard on the directory's existence (e.g., fs.existsSync or fs.statSync) and if it does not exist, call processLogger or console.error with a clear message including srcDir and exit with a non‑zero code; optionally wrap walkDir(srcDir) in a try/catch to log unexpected errors from readdirSync and rethrow/exit so failures are clear.core/loader.mjs (1)
29-50: UseaccessSyncinstead ofreadFileSyncfor existence check.
readFileSyncreads the entire file content just to check existence. UseaccessSyncfor a more efficient check:♻️ Proposed fix
-import { readFileSync } from 'fs'; +import { accessSync, constants } from 'fs'; import { fileURLToPath, pathToFileURL } from 'url'; import { resolve as resolvePath, dirname, extname } from 'path'; ... for (const ext of extensions) { try { const pathWithExt = resolvedPath + ext; - readFileSync(pathWithExt); + accessSync(pathWithExt, constants.F_OK); return { url: pathToFileURL(pathWithExt).href, shortCircuit: true }; } catch { // Try index files try { const indexPath = resolvePath(resolvedPath, 'index' + ext); - readFileSync(indexPath); + accessSync(indexPath, constants.F_OK); return { url: pathToFileURL(indexPath).href, shortCircuit: true };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/loader.mjs` around lines 29 - 50, The loop currently uses readFileSync to test for file existence (variables pathWithExt and indexPath inside the for (const ext of extensions) block); replace those existence checks with fs.accessSync (or accessSync imported from 'fs') so you don't read file contents unnecessarily: where readFileSync(pathWithExt) and readFileSync(indexPath) are called, call accessSync(pathWithExt, fs.constants.R_OK) and accessSync(indexPath, fs.constants.R_OK) respectively (keep the same try/catch flow and return the same url/shortCircuit objects) and ensure fs is imported so accessSync and fs.constants are available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/fix-imports.mjs`:
- Around line 18-28: The regexes importRegex and dynamicImportRegex contain an
unnecessary negative lookahead (?!\.js) which never matches in that position and
is redundant because the code already filters extensions with the secondary
check importPath.match(/\.\w+$/). Remove the (?!\.js) from both regexes (i.e.,
simplify /from\s+["'](\.\.?\/[^"']+?)(?!\.js)["']/g and the analogous
dynamicImportRegex) so they just capture the import path, keeping the existing
callback logic that checks importPath.match(/\.\w+$/) and appends '.js' when
needed.
- Around line 63-64: The script currently prints "Processing JS files in
${srcDir}..." and calls walkDir(srcDir) without checking that srcDir exists,
which leads to an opaque ENOENT from readdirSync; before logging and calling
walkDir, guard on the directory's existence (e.g., fs.existsSync or fs.statSync)
and if it does not exist, call processLogger or console.error with a clear
message including srcDir and exit with a non‑zero code; optionally wrap
walkDir(srcDir) in a try/catch to log unexpected errors from readdirSync and
rethrow/exit so failures are clear.
In `@core/loader.mjs`:
- Around line 29-50: The loop currently uses readFileSync to test for file
existence (variables pathWithExt and indexPath inside the for (const ext of
extensions) block); replace those existence checks with fs.accessSync (or
accessSync imported from 'fs') so you don't read file contents unnecessarily:
where readFileSync(pathWithExt) and readFileSync(indexPath) are called, call
accessSync(pathWithExt, fs.constants.R_OK) and accessSync(indexPath,
fs.constants.R_OK) respectively (keep the same try/catch flow and return the
same url/shortCircuit objects) and ensure fs is imported so accessSync and
fs.constants are available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa0871d6-1643-43aa-876f-521f4457323a
📒 Files selected for processing (3)
core/fix-imports.mjscore/loader.mjscore/package.json
lucksus
force-pushed
the
fix/mcp-admin-credential-auth
branch
from
3c940fe to
588383f
Compare
jhweir
approved these changes
Mar 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Different MCP clients provide the auth bearer in different ways. we had bugs in our auth flow. This cleans-up and properly tests the auth flow with mcporter in a new integration tests.
node upgrade to 24
tsxturbo.jsonschema updated)Summary by CodeRabbit
Improvements
Tests
Chores