PRO-1042: identify platform MCP API calls - #4
Conversation
7a8ffc9 to
1acc362
Compare
richard-lua
left a comment
There was a problem hiding this comment.
Code review — MEDIUM risk
The runtime change is a small, read-only client-identity header on the shared apiRequest wrapper — low blast radius and easily reversible. The main issue is that the header value is hardcoded to platform-mcp/1.0.0 while the new test pins it to package.json's version, so the two agree only at 1.0.0 and will diverge (and fail CI, and misreport the client version) on the next version bump. The grep-based caller guard is a reasonable idea but its regex under- and over-matches.
Major
src/api-client.mjs:38— The header is the literal'platform-mcp/1.0.0', but the added test assertsplatform-mcp/${MCP_PACKAGE.version}. On any version bump the runtime header stays at 1.0.0, the test fails, and the backend receives a stale/incorrect client version — defeating the purpose of this change. Derive the version frompackage.jsonat runtime and interpolate it into the header so the runtime value and the test's source of truth share one origin.
Minor
tests/api-client.test.mjs:84— The guard/\b(?:fetch|fetchFn)\s*\(/matches raw file text, so it can trip onfetch(inside comments/strings (false positive on unrelated edits) and misses other direct HTTP paths (http/https.request,axios,undici,node-fetch, aliased fetch) that would violate the same invariant (false negative). Strip comments/strings before matching and broaden the pattern to the HTTP mechanisms you actually want to forbid, or narrow the test's claim to match what it enforces.
PR Risk Reviewer — automated senior review of 1acc362 · risk: medium · confidence: 0.86
lua-stefan-kruger
left a comment
There was a problem hiding this comment.
Code review — HIGH risk
The runtime change is a small, well-scoped addition: a client-identity header on the shared apiRequest wrapper, plus tests that pin the header to the package version and guard against direct HTTP callers escaping api-client.mjs. The blast radius is low, but the version is hardcoded in source while the test reads it from package.json, which sets up a guaranteed drift/failure the next time the version is bumped and causes the header to misreport the real client version. Under the team's strict risk policy, this outstanding major finding makes the overall verdict HIGH — a human should weigh it before merge. The PR body's dependency on a backend parser (lua-core-services #2183) must be deployed first, as noted.
Major
src/api-client.mjs:38— The header literalplatform-mcp/1.0.0duplicates the version, but the test asserts it equalsplatform-mcp/${package.json.version}. Any version bump breaks the test and, worse, makes the header lie about the client version. Derive the version frompackage.json(JSON import assertion orfsread) instead of hardcoding it.
Minor
tests/api-client.test.mjs:81— The/\b(?:fetch|fetchFn)\s*\(/guard is brittle: it matches tokens likeprefetch(or occurrences in comments/strings, and misses direct calls viahttp.request/undici/axios. Prefer an ESLint restriction onfetchoutsideapi-client.mjs, or tighten the regex and exclude comments.tests/api-client.test.mjs:76— The identity test checks onlyX-Lua-Client; consider asserting the full header set in one place so a future header-building refactor can't silently dropAuthorization/Content-Type.
Note: also confirm the backend parser (lua-core-services #2183) is merged and deployed before this ships, per the PR description.
PR Risk Reviewer — automated senior review of 1acc362 · risk: high · confidence: 0.82
|
Resolved Stefan’s current-head findings in |
richard-lua
left a comment
There was a problem hiding this comment.
Code review — LOW risk
Previously flagged
- Previously flagged Hardcoded client version will drift from package.json and break the new test — resolved.
src/api-client.mjsnow usescreateRequire(import.meta.url)to readversionfrompackage.jsonand buildsCLIENT_IDENTITY = \platform-mcp/${PACKAGE_VERSION}`, so the runtime header and the test'sMCP_PACKAGE.version` share one source and no longer diverge on a version bump. - Previously flagged Direct-caller guard regex is brittle — resolved. The regex was replaced with a TypeScript-AST walk (
directTransportEvidence) that inspects import specifiers against aNETWORK_MODULESset and call expressions forfetch/fetchFn/*.fetch, eliminating the comment/string false positives and covering the common HTTP libraries.
Both prior issues are fixed and the underlying change is a small, read-only client-identity header on the shared apiRequest wrapper. Remaining notes are minor test-robustness observations, not blockers. Merge order still depends on the backend parser PR called out in the description.
Minor
tests/api-client.test.mjs:10— The guard test now imports and runs the TypeScript compiler API. Fine as a devDependency in CI, but it widens the test toolchain's failure surface (missing dev deps or a TS default-export interop change would fail this test). Optionally document the dependency or guard the import with a clear message.tests/api-client.test.mjs:45— The AST guard still misses dynamicimport('undici')/require('node:http'),globalThis.fetch(...)accessed without a.fetchcall site, andXMLHttpRequest/WebSocket. Much stronger than the old regex and sufficient for today's code, but the test's stated invariant is broader than what it enforces. Extend the detection or narrow the comment.
PR Risk Reviewer — automated senior review of 64c65a4 · risk: low · confidence: 0.85 · re-review
lua-stefan-kruger
left a comment
There was a problem hiding this comment.
Code review — LOW risk
Previously flagged
- Previously flagged Hardcoded client version will drift from package.json — resolved:
src/api-client.mjsnow derivesPACKAGE_VERSIONfromrequire('../package.json')and builds the header from it, so the header and test share one source of truth. - Previously flagged Direct-caller guard regex is brittle — resolved: the regex is replaced by a TypeScript-AST scan that checks import specifiers against a network-module set and call expressions for
fetch/fetchFn/.fetch, avoiding comment/string false positives and catching axios/got/ky/undici/node:http imports. - Previously flagged Header-identity test does not assert Authorization/Content-Type coexist — resolved: the new identity test asserts the full header object via
toEqual, so dropping any header would fail.
All prior findings are addressed. The remaining items are advisory and non-blocking; blast radius is small and the change is well-tested.
Minor
src/api-client.mjs:11— The runtimerequire('../package.json')at module load must resolve in the published/bundled artifact. The package builds todist/server.jsvia esbuild and ships onlydist/; if the JSON isn't inlined or isn't adjacent at the expected relative path, startup throws for every tool call. The source-level tests wouldn't catch a bundling regression. Confirm esbuild inlines the JSON import (default behavior) or add a smoke test that loadsdist/server.js.tests/api-client.test.mjs:44— The AST guard is a strong improvement but still won't catchglobalThis['fetch'](), computed member access, or transports likeWebSocket/http2. Consider backing it with an ESLintno-restricted-globals/no-restricted-importsrule scoped outsideapi-client.mjsfor uniform enforcement.
Note: per the PR body, confirm the backend parser (lua-core-services #2183) that recognizes platform-mcp is merged and deployed before this ships.
PR Risk Reviewer — automated senior review of 64c65a4 · risk: low · confidence: 0.83
PR Risk Reviewer — automated senior review of 64c65a4 · risk: low · confidence: 0.83 · re-review
What changed
X-Lua-Client: platform-mcp/1.0.0on direct Lua API requests.src/**/*.mjsfile in the test and fail if a direct HTTP caller appears outsidesrc/api-client.mjs.Lua CLI subprocesses are outside this wrapper and keep their own
cli/<lua-cli version>identity.Dependency
This draft depends on lua-core-services #2183, which adds
platform-mcpto the bounded server parser. Merge and deploy the backend parser first.This branch starts from current
main. It does not modify or replace outage re-review PR #3.Verification
npm test -- --runTestsByPath tests/api-client.test.mjs(15 tests)npm run buildPRO-1042