Skip to content

fix: sweep table-cell escaping (CodeQL #74-#76) and document mcpdo's environment variables - #2641

Merged
cliffhall merged 5 commits into
v2/mainfrom
v2/fix/2546-sweep-cell-escaping
Oct 7, 2026
Merged

cliffhall merged 5 commits into
v2/mainfrom
v2/fix/2546-sweep-cell-escaping

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #2546
Closes #2640

These are the two remaining items from the v2.10.0 milestone merge, PR #2637 (#2623): CodeQL fails on #74–#76, and Copilot's second round flagged the environment-variable docs.

#2546: CodeQL #74–#76 (9c88e307, plus a Prettier reflow)

dependabot-alerts.mjs (twice) and sdk-watch.mjs escaped | in Markdown table cells but not \, so a value ending in a backslash re-exposed the pipe. A new shared helper, scripts/lib/markdown-cell.mjs escapeTableCell, escapes backslashes first, then pipes, and both scripts import it, so the three copies are gone. Its sibling test covers a trailing backslash and asserts that every pipe in the output sits behind an odd run of backslashes. The tests for the helper and both scripts pass, 155/155.

#2640: mcpdo in docs/environment-variables.md (7a2c154e)

  • A new mcpdo connection daemon section documents MCP_INSPECTOR_DAEMON_DIR, MCP_INSPECTOR_DAEMON_TOKEN and MCP_ALLOW_DEFAULT_CONNECTION. Defaults and effects are taken from daemon/paths.ts, daemon/auth.ts and connection/dispatch.ts. These were documented only in the spec.
  • mcpdo is named in the Read-by legend and in the 16 rows it reaches through core/. Each one was checked against the built mcpdo bundle: the variable name appears in it. The proxy rows are reached through EnvHttpProxyAgent in core/mcp/node/proxyFetch.ts, which core/mcp/node/transport.ts uses. Variables absent from the bundle (HOST, the ports, logging) are not marked.

Verification

npm run local:gate: every stage passes (scripts 996, format coverage 1448 files, DCO 2/2, smoke:mcpdo passed) except smoke:web:firefox, which cannot launch Playwright's Firefox on macOS 27 (#2625). local:storybook: 529 passed.

🤖 Generated with Claude Code

cliffhall and others added 3 commits October 7, 2026 17:30
…2546)

dependabot-alerts.mjs (twice) and sdk-watch.mjs escaped | in Markdown
table cells but not \, so a value ending in a backslash re-exposed the
pipe and broke the row (CodeQL js/incomplete-sanitization #74-#76).

One shared helper, scripts/lib/markdown-cell.mjs, escapes backslashes
first, then pipes; both scripts import it. Its test covers a trailing
backslash and checks every pipe in the output sits behind an odd run of
backslashes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
…2640)

docs/environment-variables.md claims every runtime variable but never
mentioned mcpdo, which ships for the first time in 2.10.0.

- A new 'mcpdo connection daemon' section documents
  MCP_INSPECTOR_DAEMON_DIR, MCP_INSPECTOR_DAEMON_TOKEN and
  MCP_ALLOW_DEFAULT_CONNECTION from daemon/paths.ts, daemon/auth.ts and
  connection/dispatch.ts; they were only in the spec.
- mcpdo is named in the Read-by legend and in the 16 rows it reads
  through core/: each name was checked against the built mcpdo bundle
  (the proxy rows through EnvHttpProxyAgent in core/mcp/node/proxyFetch).

Raised by Copilot on the v2.10.0 milestone merge (#2637).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The documentation incorrectly promises mcpdo proxy support and implies support for unavailable command-line flags.

3 open findings
What changed in this PR

Addresses two release follow-ups: shared Markdown table-cell escaping for automation scripts and expanded mcpdo environment-variable documentation.

Changes:

  • Centralizes backslash and pipe escaping across both dependency sweeps.
  • Adds regression tests for escaping and non-string inputs.
  • Documents mcpdo daemon variables and shared configuration readers.
File Description
scripts/​sdk-watch.mjs Uses the shared cell escaper.
scripts/​lib/​markdown-cell.test.mjs Tests escaping edge cases and coercion.
scripts/​lib/​markdown-cell.mjs Escapes backslashes before pipes.
scripts/​dependabot-alerts.mjs Replaces duplicated escaping logic.
docs/​environment-variables.md Adds mcpdo configuration documentation.

🧠 Review effort: Balanced


💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread docs/environment-variables.md Outdated
Comment thread docs/environment-variables.md Outdated
Comment thread docs/environment-variables.md Outdated
…lags (#2640)

Copilot on #2641, verified in source:
- mcpdo does not honour HTTPS_PROXY/HTTP_PROXY/NO_PROXY. Its two
  InspectorClient environments (connection/authorize.ts,
  daemon/connections.ts) omit fetch, so InspectorClient wraps global
  fetch (inspectorClient.ts:846) and the transport never reaches
  createProxyFetch(). The variable names are in the mcpdo bundle, but
  that code is never called on these paths.
- mcpdo has no --callback-url or --client-config flag; say the flag
  overrides apply to the CLI and TUI only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The escaping fix has focused regression coverage, and remaining feedback concerns minor documentation clarifications.

0 open findings

3 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Low severity Document mcpdo's non-interactive OAuth behavior

docs/​environment-variables.md:50

mcpdo is now listed, but the effect omits its distinct non-interactive behavior. When a connection needs OAuth, neither stdin nor stderr is a terminal, and --stored-auth-only is not set, mcpdo returns a pending connection and authorization URL rather than the CLI's auth-required error. Setting MCP_AUTO_OPEN_ENABLED=true instead keeps the blocking interactive flow (clients/mcpdo/src/connection/mcp.ts:687–724). Document this distinction for scripted callers.

Low severity Document daemon directory fallback to os.homedir()

docs/​environment-variables.md:70

The new daemon-directory default does not follow this blanket current-directory fallback. With no path override and neither HOME nor USERPROFILE set, getDaemonDir() uses os.homedir() (clients/mcpdo/src/daemon/paths.ts:23–30). Note that exception so users running under service managers look for the daemon socket and lock in the correct directory.

🧠 Review effort: Balanced

…llback (#2640)

Copilot round 2 on #2641, verified in source:
- MCP_AUTO_OPEN_ENABLED: with no TTY and no --stored-auth-only, mcpdo
  connect returns a pending connection and authUrl instead of the CLI's
  auth-required error; true keeps the blocking flow
  (connection/mcp.ts).
- USERPROFILE: the daemon directory falls back to os.homedir(), not the
  working directory, when neither HOME nor USERPROFILE is set
  (daemon/paths.ts).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Round 2 raised two notes outside any thread, both on rows this PR touched. I verified each in source and fixed both in 337d5b1b:

  • MCP_AUTO_OPEN_ENABLED (line 50): the row now describes mcpdo's non-TTY OAuth hand-off. With no TTY on stdin or stderr and no --stored-auth-only, connect exits 0 with a pending connection and an authUrl to relay; true keeps the blocking flow (connection/mcp.ts:687–724).
  • USERPROFILE (line 70): the row now notes that the mcpdo daemon directory falls back to os.homedir(), not the working directory, when neither HOME nor USERPROFILE is set (daemon/paths.ts:23–30).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

Only minor documentation and file-header nits remain; no blocking defects were identified.

0 open findings

🧠 Review effort: Balanced

@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review loop closed. Round 3 recommends approval with 0 open findings: no inline comments, and no suppressed block. Its headline mentions unspecified "minor nits", but none are itemized anywhere, so there is nothing actionable. Round 1's three findings and round 2's two notes are all fixed, and the mcpdo proxy gap is filed as #2642.

@cliffhall
cliffhall merged commit 643df82 into v2/main Oct 7, 2026
8 checks passed
@cliffhall
cliffhall deleted the v2/fix/2546-sweep-cell-escaping branch October 7, 2026 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

2 participants