feat: publish standalone GitHub release bundle - #5
Conversation
PR #5 Review:
|
| Issue ID | Issue name | Category | Severity | Location | Description | Steps to reproduce | Suggested fix |
|---|---|---|---|---|---|---|---|
| #1 | Historical bundle URLs fail | Trust boundaries & security | Moderate | in-diff — src/pairingTools.ts:157, src/distribution.ts:112 |
PR #5 introduces the first standalone asset at 0.4.0, but every canonical version is treated as having one. Version-mismatch responses can mandate downloading nonexistent 0.2/0.3 assets. Tests explicitly preserve this incorrect assumption. | Run standalone 0.4.0 and pair with a console expecting 0.2.0. Pairing succeeds because both use major 0, but the returned upgrade URL targets v0.2.0/mcp-server-questdb-0.2.0.mjs, which does not exist. |
Model standalone availability from 0.4.0 onward. For earlier versions, use the appropriate npm package or return honest unsupported/offline guidance. Correct the tests. |
| #2 | Windows Codex path breaks | Tests & coverage | Moderate | out-of-diff — src/setup/codexCli.ts:13; arbitrary standalone-path contract introduced by src/distribution.ts:28 |
Standalone paths now reach the existing Windows cmd.exe quoting layer. winQuote does not quote ( or ), so legal no-space paths containing parentheses are parsed as shell syntax and Codex setup or upgrade fails. |
Run the bundle from C:\QuestDB(test)\mcp-server-questdb-0.4.0.mjs, then configure Codex. The path remains unquoted in codex mcp add ... -- node <path>. |
Avoid shell:true and custom quoting if possible. Otherwise quote every cmd.exe metacharacter, including parentheses, and add an injected Windows-path test. |
| #3 | Reconnect cancels foreign grace | Disconnect, reconnect & races | Moderate | out-of-diff — src/bridgeSession.ts:599; per-call reconnect-grace ownership contract |
A reconnect cancels grace for every in-flight call, including calls belonging to an earlier console. A lost long-running call then waits up to five minutes instead of returning the 30-second unverified-disconnect warning. | Console A starts run_query and disconnects. Fresh console B pairs, disconnects, then reconnects with B’s ID. The reconnect loop cancels A’s grace timer, leaving A’s call pending until its 300-second deadline. |
Store the owning session ID on each in-flight call and cancel grace only for calls owned by the reconnecting session. |
| #4 | Failed reconnect erases identity | Disconnect, reconnect & races | Moderate | out-of-diff — src/bridgeSession.ts:743; detached-session preservation contract |
An S0 handshake failure overwrites detachedSessionId with null. A subsequent valid reconnect is no longer recognized, so its call falsely returns browser_disconnected even though the original console returned. |
Console A starts a long query and disconnects. Its first reconnect drops before hello, overwriting the detached ID with null. Its second reconnect succeeds, but grace is not canceled; at 30 seconds the call fails and a later legitimate result is dropped. |
Do not overwrite the detached identity on S0→S0 failures. Prefer per-call session/grace ownership and add a fake-timer regression test. |
| #5 | Hidden tools bypass surface | MCP server contract & tool surface | Moderate | out-of-diff — src/mcpServer.ts:162; static/live tool allowlist contract |
The SDK accepts any string as a tool name, and the bridge forwards every non-pairing name without checking BUNDLED_TOOL_NAMES. Calling AI-only suggest_query bypasses the MCP surface and live schema validation; the Console returns success without displaying a suggestion. |
Pair normally, then send a raw tools/call for suggest_query. It is absent from tools/list, but reaches the browser’s generic dispatcher and returns a false success. |
Enforce BUNDLED_TOOL_NAMES before forwarding and return UNKNOWN_TOOL with isError:true. Optionally require membership in the live advertised set too. |
| #6 | Lifecycle comment is false | Session state machine & protocol | Minor | in-diff — src/bridgeSession.ts:749 |
The new comment says incompatibleConsole is cleared only by a successful hello, but beginPairingAttempt and fresh attachBrowser also clear it. This documents a false lifecycle invariant in the highest-risk state machine. |
Cache a major mismatch, let its socket close, then call get_pairing_credentials or attach a fresh authenticated socket. The cache clears without a successful hello. |
Update the comment to enumerate successful hello, explicit retry, and fresh attach. |
False-positives
| Category | Description | Explanation |
|---|---|---|
| Timers, in-flight calls & cancellation | A throwing send(cancel) leaves deadline or abort promises unsettled. |
Production’s adapter skips non-open sockets, uses a fixed JSON-safe frame, and ws.send cannot synchronously throw on the reachable open-state path. The sequence requires an artificial throwing adapter. |
| Session state machine & protocol | A malformed expectedBridgeVersion strands waiters and causes reconnect loops. |
Supported Console builds send a hard-coded canonical version. Protocol violations intentionally leave waiters available for a later valid connection; only actionable major mismatches terminate and drain them. |
| Timers, in-flight calls & cancellation | A fresh authenticated socket incorrectly erases a cached version diagnosis. | This is intentional new-attempt behavior. Retaining the predecessor’s mismatch would immediately reject waiters while a potentially compatible replacement is handshaking. |
Summary
Approve.
There are no Critical findings. The in-diff findings are one Moderate standalone-recovery defect and one Minor lifecycle-comment defect, so they do not block this PR under the requested approval policy. The disconnect-grace/no-duplicate-DML review also identified two pre-existing out-of-diff ownership problems.
Eight remaining unique draft findings were verified: five confirmed and three dropped as false positives. Verification additionally found one Minor in-diff comment defect. The final split is two in-diff issues and four out-of-diff issues.
Typecheck, build, lint, and all 313 tests pass.
Review findingsImportant — standalone mismatch guidance links to nonexistent historical bundlesLocations: Standalone release assets begin with v0.4.0, but the implementation treats every canonical version—including 0.2.x and 0.3.x—as downloadable. A v0.4 standalone bridge paired with a console expecting v0.2/v0.3 directs the user to a GitHub asset that returns 404. Model standalone availability from v0.4.0 onward (or use an explicit manifest), and provide npm/console-upgrade guidance when no historical standalone asset exists. Important — MCP Registry recovery is not target-version idempotentLocation: The existence check compares the target only with Use the version-specific endpoint Important — legal Windows standalone paths can break Codex configurationLocations: Standalone bundle paths now pass through the Windows Minor — bundle E2E environment is not fully isolatedLocation:
Validation
Assessment: Not ready to merge until the Important findings are addressed. |
2 participants
Summary
.mjsbridge and third-party notices for GitHub ReleasesThe standalone bundle and the npm build use the same MCP server implementation. The currently published npm 0.4.0 packages remain unchanged; this workflow will detect them and complete the npm stage as a no-op.
Validation
yarn install --immutableyarn typecheckyarn lintyarn buildyarn test— 309 tests passedyarn bundleactionlintserver.jsonpasses the live MCP Registry publisher validationExpected first run
v0.4.0, build and publish the standalone bundle plus noticesKnown non-blocking limitation
On a minimal Linux desktop without a system
xdg-open, automatic browser opening from the single-file bundle can fail; the pairing response still provides the deep link and credentials, so manual pairing continues to work.