potiuk commented on code in PR #55:
URL: https://github.com/apache/comdev/pull/55#discussion_r4315851171
##########
mcp/ponymail-mcp/package.json:
##########
@@ -10,7 +10,7 @@
"licenses": "node ../../scripts/check-licenses.mjs ."
},
"engines": {
- "node": ">=20"
+ "node": ">=22.21"
Review Comment:
`package-lock.json` still records `"engines": {"node": ">=20"}` for the root
package. Could you run `npm install --package-lock-only` here and commit the
result? #56 adds a prek hook (`package-lock-sync`) that catches this at commit
time.
##########
.github/workflows/mcp-tests.yml:
##########
@@ -27,7 +27,7 @@ jobs:
fail-fast: false
matrix:
project: ["ponymail-mcp", "apache-projects-mcp"]
- node-version: ["20", "22"]
+ node-version: ["22"]
Review Comment:
`["22"]` resolves to latest 22.x, so neither the declared floor nor Node 24
LTS is exercised.
```suggestion
node-version: ["22.21", "24"]
```
##########
mcp/apache-projects-mcp/package.json:
##########
@@ -9,6 +9,9 @@
"test": "node --test",
"licenses": "node ../../scripts/check-licenses.mjs ."
},
+ "engines": {
+ "node": ">=22.21"
Review Comment:
Same here — `package-lock.json` has no `engines` entry for the root package
yet; `npm install --package-lock-only` will add it.
##########
mcp/ponymail-mcp/index.js:
##########
@@ -43,6 +43,18 @@ import {
} from "./restrictions.js";
import { Mail, extractListDomain, countDescendants } from "./mail.js";
+// Node's built-in fetch only uses proxy variables when NODE_USE_ENV_PROXY=1
+// is set at startup, so a proxy variable alone would be silently ignored.
+if (
+ ["HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy"].some((v) =>
process.env[v]) &&
+ !process.env.NODE_USE_ENV_PROXY
+) {
Review Comment:
`!process.env.NODE_USE_ENV_PROXY` treats `NODE_USE_ENV_PROXY=0` as enabled
(no warning, proxy not used), and flags a false warning when the feature is
enabled via `--use-env-proxy` on the CLI or in `NODE_OPTIONS`:
```suggestion
if (
["HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy"].some((v) =>
process.env[v]) &&
process.env.NODE_USE_ENV_PROXY !== "1" &&
![...process.execArgv, ...(process.env.NODE_OPTIONS ??
"").split(/\s+/)].includes("--use-env-proxy")
) {
```
Separately: `engines` is advisory, so on Node < 22.21 the variable is
silently ignored and no warning fires — a Node version check might be worth
adding.
##########
mcp/apache-projects-mcp/README.md:
##########
@@ -90,6 +90,22 @@ Example stdio configuration:
Adjust the path to match your local checkout.
+### Behind a proxy
+
+Node's built-in `fetch` only honors `HTTPS_PROXY` / `HTTP_PROXY` / `NO_PROXY`
+when `NODE_USE_ENV_PROXY=1` is set at process startup. Set both in the
+server's `env` block:
+
+```
Review Comment:
Nit: missing `json` tag (the ponymail README has it).
````suggestion
```json
````
##########
mcp/apache-projects-mcp/index.js:
##########
@@ -746,6 +746,18 @@ server.tool(
// ---------------------------------------------------------------------------
if (process.argv[1] && import.meta.url ===
pathToFileURL(process.argv[1]).href) {
+ // Node's built-in fetch only uses proxy variables when NODE_USE_ENV_PROXY=1
+ // is set at startup, so a proxy variable alone would be silently ignored.
+ if (
+ ["HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy"].some((v) =>
process.env[v]) &&
+ !process.env.NODE_USE_ENV_PROXY
+ ) {
Review Comment:
Same as in ponymail-mcp:
```suggestion
if (
["HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy"].some((v) =>
process.env[v]) &&
process.env.NODE_USE_ENV_PROXY !== "1" &&
![...process.execArgv, ...(process.env.NODE_OPTIONS ??
"").split(/\s+/)].includes("--use-env-proxy")
) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
