Conversation
Groundwork for validating what this server advertises to MCP clients.
Checking the whole tool catalog from a test means the spawned server has
to register exactly the tools it would in production, and today every
test that starts one re-implements the spawn and spreads the whole of
process.env into the child. A stray TOOLS or GROUPS in a developer's
shell therefore silently shrinks the catalog under the test.
test/helpers/spawn_server.js owns that contract instead:
- an explicit inherit list. The MCP SDK already supplies HOME, PATH,
SHELL, TERM and USER, so this carries the network plumbing a
developer behind a proxy depends on, and nothing else.
- network: 'none', which points the child at a refused proxy for tests
that must never reach the real API.
- captured, drained stderr, so a test can assert on what the server
reported. Draining is not optional: a piped stderr nobody reads
blocks the child once the pipe buffer fills, and this server logs
every tool call.
- a deployed-server variant that sends the token in an Authorization
header rather than in a URL, and scrubs the exact value from
anything it throws, so a transport error cannot put a credential in
a terminal or a CI log.
The npm script now names test/*.test.js. Bare `node --test` treats every
.js file under test/ as a test, which would run this helper as an empty
passing test. The pattern stays single-level deliberately: under sh
without globstar, test/**/*.test.js matches only files below test/ and
would silently drop the entire suite.
…rtup zone check Diagnostics were 26 bare console.error calls across two files. Eight had hand-written [scope] prefixes, the rest had none, so "did the server warn about this?" could only be asked by matching free text, and an operator running the server behind a wrapper had no way to filter or quiet them. logger.js gives every line a scope tag and a LOG_LEVEL threshold (silent|error|warn|info|debug, default info). Levels gate emission only: each message is printed exactly as the call site wrote it, printf placeholders included, so the lines this replaces are unchanged apart from the tag. Everything stays on stderr, because on the stdio transport stdout carries the MCP protocol itself and a stray write there corrupts the session. Scopes: [zone] for the startup bootstrap, [server] for lifecycle, [browser] for session handling, [config] for configuration problems, and the tool's own name for per-call lines. The zone bootstrap also gets the bound it never had. It runs before the MCP handshake with no timeout, so a proxy that drops packets rather than refusing them freezes startup with nothing diagnosable on the client side: the client waits for a handshake that never comes. It now honors BASE_TIMEOUT, else ten seconds. Unlike a tool call, "no timeout" is never a sensible setting for startup, so the bound applies even when BASE_TIMEOUT is 0. This change is byte-identical to the one in brightdata#176, so the two merge cleanly in either order and whichever lands second is a no-op there.
This is the tool-annotation half of preparing the server for OpenAI's
ChatGPT and Codex plugin directory. Their submission portal scans the
live MCP endpoint, reads readOnlyHint, openWorldHint and
destructiveHint from every tool, and checks them against OpenAI's
published definitions; hints that are missing or contradicted by what
the tool does are a documented rejection reason. All 74 tools this
server advertises currently fail that check.
The hints matter beyond that review. Clients use them to decide when to
ask a user for confirmation, and absent hints are not neutral: the MCP
specification defaults destructiveHint and openWorldHint to true, so a
partially annotated tool reads as more dangerous than it is. A browser
"go back" was advertised as destructive and open-world, while filling in
a form on an arbitrary website looked like taking a screenshot.
What was wrong, by those definitions:
- 51 tools that start a billed collection job (the 50 web_data_* tools
and discover) claimed readOnlyHint: true. The definition sets it
false for anything that can "run jobs, start workflows ... or
otherwise change state". They create a snapshot in the user's own
account that cannot be un-created, but they destroy, overwrite and
send nothing, so destructiveHint stays false.
- navigate, go_back, go_forward, scroll and scroll_to_ref claimed
destructiveHint: true although every one of them is reversible.
- click_ref and type_ref can submit a form on a third-party site, the
textbook case for openWorldHint, and carried none.
- list_dataset_fields and search_dataset talk only to Bright Data's own
API and claimed openWorldHint: true.
- fill_form carried no annotations at all, not even a title.
tool_annotations.js replaces 25 hand-written objects with eight named
classes, so a tool declares what kind of thing it is and the class
supplies all three hints. That is what stops the drift this commit is
fixing: the values cannot be partially written, and a reviewer changing
a policy edits one line rather than fifty-one.
annotate() never throws. An unknown class falls back to the most
conservative hints, warns on [config], and marks the title
[unclassified]. The mark is load-bearing: that fallback triple is
identical to the browser_act class, so without it a mistyped class on
click_ref or type_ref would satisfy every value check and ship as a
warning nobody reads.
test/annotations-coverage.test.js imports none of that, on purpose. It
lists the tools a client actually sees and asserts completeness, that
the advertised set equals the classified set in both directions, and
that each tool carries its class's literal values. Two source invariants
keep annotations written only through the helper and only with known
class names. Given MCP_URL and MCP_TOKEN it runs the same checks against
a deployed server, which is what the submission portal scans and what a
merged-but-not-yet-released change would otherwise leave unverified.
Tool names, descriptions and input schemas are byte-identical before and
after; only annotations move.
This was referenced Sep 15, 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
1 participant
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.
Why
OpenAI's ChatGPT/Codex plugin directory imports each tool's MCP annotations (
readOnlyHint,destructiveHint,openWorldHint) when it scans a server, and rejects submissions whose annotations do not match what the tool does. On main every one of the 74 tools is missing at least one of the three, and several of the values that are present are wrong:scraping_browser_go_backclaims to be destructive, whilescraping_browser_type_ref, which types into a form on a third-party site, claims nothing at all. The MCP spec defaults a missingdestructiveHintandopenWorldHinttotrue, so a scanner reading main today sees 74 tools that all claim to be destructive.What changes
Three commits, each usable on its own.
test/helpers/spawn_server.jsspawns the server with a hermetic environment (no inheritedTOOLS/GROUPS; optionalnetwork: 'none'that points the child at a refusing proxy so startup finishes in well under a second without touching the API).npm testnow runstest/*.test.jsrather than every.jsundertest/, which barenode --testwould otherwise pick up. This commit is shared verbatim with fix: keep credentials and debug payloads out of tool results (complement to 2.11.2) #185 and feat: send the model server instructions on how to escalate between tools #186; whichever merges first, the others drop it on rebase.logger.js([scope] messageon stderr,LOG_LEVELthreshold) instead of 26 hand-formattedconsole.errorcalls. The zone check at startup gets the same timeout as every other request (BASE_TIMEOUT, default 10s), so a hung API cannot freeze the server before it answersinitialize. That timeout change is byte-identical to fix: bound the startup zone check so a hung API cannot freeze the server #176, which this PR supersedes.tool_annotations.jsdefines the classes (sync_fetch,job_start,closed_read,browser_read,browser_navigate,browser_scroll,browser_act,browser_fill), each with its three hints and a written reason. A tool declares its class and the class supplies the values; the 50 generatedweb_data_*tools get theirs from one place. An unknown class never throws: it falls back to the conservative triple, logs a[config]warning, and suffixes the title with[unclassified]so the mistake is visible in any client.One decision for maintainers
The 51 tools that start a billed collection job (
web_data_*,search_dataset, the marketplace queries) are markedreadOnlyHint: false. That follows the MCP definition, since they create a job and spend credits, but some clients ask the user to confirm before calling a tool that is not read-only. Whether ChatGPT does so has not been observed; the class design makes flipping it a one-line change injob_start.Two tools are marked destructive on purpose,
scraping_browser_click_refandscraping_browser_type_ref, because a click or a keystroke on a third-party page can submit, purchase or send. The reasons are written into the class table.Verification
npm test: 45 tests, 44 pass, 1 skipped. The skipped one checks a deployed server whenMCP_URLandMCP_TOKENare set; the token travels in the header, never the URL.test/annotations-coverage.test.jsasserts that every registered tool carries all three hints, that every tool is in a golden class map, that no source file writesannotations: {by hand, and that startup emits no[config]warning.Not verified
readOnlyHint: falsetools. Needs a session in ChatGPT developer mode.mcp.brightdata.com, a separate build reporting version 1.0.0, will carry these annotations once released. The directory scans the deployed server, not this repository.