feat(cli): configure MCP natively across coding agents - #187
Open
Max17190 wants to merge 35 commits into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Replace the subprocess installer in `setup mcp` with a built-in one that detects installed agents, pre-selects them in a picker, and offers to install rules telling those agents to prefer Firecrawl for web search and scraping. Covers Claude Code, Cursor, VS Code, Codex, OpenCode, Windsurf, Zed, Hermes Agent, and OpenClaw through per-agent flags, `--project` for project scope, and `--rules` / `--no-rules` for scripted runs. `-y` stays MCP-only. The two launchers were previously reachable only by flag, so a plain `setup mcp` never offered them; they now sit in the picker alongside the editors, and because a launcher shells out to a CLI, a missing binary is reported against that one agent instead of ending the run. Credential handling is unchanged in principle and stricter in reach: an API key is never written as a literal. Knowing which agents were selected means each one receives a reference to FIRECRAWL_API_KEY in the syntax it expands, so the setup no longer has to refuse a run that omits --agent. Agents with no verified syntax fall back to the keyless endpoint and say so rather than persisting a secret. Config edits are surgical. JSON is patched through a JSONC-aware editor so commented settings files parse at all and keep their comments, and TOML tables are replaced along with any stale sub-tables left by a previous stdio entry. Reruns are byte-identical. Also gives every setup test a throwaway HOME and resets spawn mocks between tests, since MCP setup now writes real config files and would otherwise rewrite the developer's own agent settings; and teaches doctor about the `servers` and `context_servers` keys so those registrations are recognized.
The PATH lookup matched any executable named `hermes`, including an unrelated JavaScript engine that ships with common toolchains, so the picker pre-selected an agent the user did not have. Detection now prefers a false negative to a false positive: every agent is listed either way, so missing one costs a keystroke while pre-selecting a missing one is misleading. Also pins HOME and PATH for setup tests. Both feed agent detection, so leaving the real ones visible made results depend on what happened to be installed on the machine running the suite.
Zed's native remote MCP support is version-gated and its handling of request headers is inconsistent across releases, so a written entry can report success while the agent never connects. That reads as Firecrawl being broken, which is worse than not offering the agent at all. Removing it until the shape can be confirmed against a live install. Also pins the picker page size to the number of agents. The default was smaller than the list, so the last agent scrolled out of view.
Windsurf's remote entry shape is not settled: sources disagree on whether a transport field is required and what its value should be, and one reports streamable HTTP working only through a local proxy. A wrong entry does not error, it reports success and then exposes no tools, so this stays out until the shape can be confirmed against a live install. With every supported agent now carrying a verified environment-reference syntax and project-level config, the keyless-fallback and global-fallback branches no longer have a case. Removing them rather than leaving unreachable logic behind; they come back with the agent that needs them.
Six defects, four of them silent: * Quiet mode returned before the total-failure check, so a run in which nothing was written resolved successfully. `firecrawl init` and `firecrawl launch` both use quiet mode and reported success regardless. * The TOML writer split on "\n" only, so a config.toml with CRLF endings never matched its existing table and gained a duplicate one, leaving the file invalid and taking the rest of the user's Codex config with it. * The TOML writer absorbed comment and blank lines directly above the next table into the replaced range and deleted them. * A leading byte order mark was reported as a parse error even though the document parses, so a config written by a Windows editor was refused. * The rule fence required "\n" after its marker, so a file converted to CRLF gained a second copy of the section instead of an updated one. * `--agent all` reached only detected clients. It means every client, which is what the installer it replaced did. The fence replacement now uses a function so nothing in the rule body can be read as a replacement pattern. Line endings and byte order marks are preserved on write rather than normalised away.
…scope flag `firecrawl setup --yes --agent windsurf` installed skills and then aborted, because MCP setup rejected a name it writes no config for. An agent we support for skills but not for MCP is not an error: the run now finishes, skips the MCP step, and prints the server URL so the user can wire it up themselves. A name nothing supports is still rejected, so a typo does not silently do nothing. Scope: global is the intended default, so that one command reaches every agent surface rather than the current checkout alone. `--project` is the only scope flag that means anything on setup. `-g` is accepted for existing scripts but hidden from help and reported as deprecated when used, and the mutually exclusive scope error it existed for is gone. `-g` is untouched on init and launch. Tests also pin USERPROFILE and APPDATA alongside HOME. os.homedir() reads USERPROFILE on Windows, so the sandbox that keeps a test run away from the developer's own agent config was doing nothing there.
Max17190
force-pushed
the
feat/native-mcp-setup
branch
from
August 12, 2026 21:55
cc7e59b to
9162131
Compare
…keys Project scope fought the one-command-every-agent goal, and --agent hermes/openclaw aborted on a stored key while the boolean flags wrote keyless config.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
… artifacts Three Firecrawl MCP configs were tracked at the repository root. Tests wrote them into the working directory and a broad `git add` swept them in, so anyone opening this checkout inherited MCP servers from the repo. Removed, and every setup test now runs in its own working directory so project-relative writes cannot reach the repository again. A stored key behaved three different ways depending on how the target was named: keyless for the boolean flags, keyless for `--agent hermes`, and a hard abort for `--agent all`. The README documents keyless. `--agent all` now agrees with the rest, and the launchers report through the same summary so the keyless fallback is stated rather than implied by a bare installer log line. `--agent launchers` was a synonym for every agent plus both launchers, which is not what the name says. It now selects the launchers. Launcher dispatch is exhaustive rather than treating anything that is not Hermes as OpenClaw, and doctor recognises OpenCode's top-level `mcp` map, which it previously reported as unregistered right after setup wrote it.
Four defects, each reported against this branch and each confirmed against the code before changing it: * `resolveMcpClientId` read `__proto__` and `constructor` off the alias object's prototype and returned something truthy, so those two names resolved to a bogus agent and crashed the run instead of being rejected as unknown. `toString` and the rest are already rejected because lowercasing them stops matching an inherited key. * VS Code was detected only through `Code/User`, which is created on first launch. The picker offers detected agents alone, so an install that had not been launched was invisible rather than merely unselected. Detection now uses the same two markers doctor already uses. * The TOML editor scanned lines for table headers with no awareness of multi-line strings, so a `[table]` written inside one was treated as a real header and the surrounding edit could corrupt the file. The scan now tracks string state, and a multi-line string left open at the end of the file is reported as a per-agent failure instead of being appended to, which matches how the JSON path already treats a config it cannot parse. * `hasFirecrawlMcpEntry` matched `servers` and `mcp` at any depth, so an unrelated nested object holding a `firecrawl` property made doctor report the server as registered. Those two keys only ever sit at the root of the configs that are scanned; `mcpServers` keeps matching at any depth because Claude Code nests a per-project map under `projects`. Full TOML validation is deliberately not attempted. It would need a parser dependency, and a line-based validator would reject valid configs, since a TOML array may legally span several lines.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The scanner added in the previous commit closed a multi-line basic string at the first `"""` it found, but a basic string honours backslash escapes, so `\"""` is an escaped quote followed by two literal ones rather than the terminator. Ending the string there marked the rest of it as ordinary config, which is how a `[table]` written inside a string becomes a header the editor will replace, taking the user's content with it. Fence candidates are now skipped while the backslash run before them is odd. An even run is a real terminator, since backslashes escape each other. Literal strings are unaffected because they have no escapes at all.
Hermes was classified as a launcher, which implied it owns its MCP config and
has to be shelled out to. It does not: it reads plain YAML from
~/.hermes/config.yaml under `mcp_servers`, with `url` and a `headers` mapping
for an HTTP server, and it expands `${VAR}` in any string value in a server
entry. All of that is documented by Nous Research and matches what we already
emit, so Hermes is a config-file client and now goes through the same engine as
the editors.
That fixes a real defect. The old writer round-tripped the file through
parse/stringify, so a hand-written config.yaml came back with every comment,
inline note, and blank line removed. Edits now go through the YAML document
tree, which keeps comments, key order, and formatting, and a file that does not
parse is reported as a per-agent failure instead of being rewritten. A config we
create is owner-only; one the user already has keeps the permissions they gave
it, rather than being chmod-ed on every run.
Hermes gets no rules. It reads AGENTS.md from the project directory and setup
only ever writes global config, so there is no global rule file to own, and the
summary says so rather than implying one was written.
OpenClaw stays a launcher, and the type now says why. Its config is JSON5, which
the JSONC editor we patch JSON with cannot read, so writing that file directly
would either corrupt it or refuse a valid config. `openclaw mcp set` is the
vendor-documented path and normalises the entry on the way in.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Two defects in the new Hermes writer, both reported against the last commit: * A server key with nothing under it, `mcp_servers:` on its own, parses as a null scalar. Setting a path through that refuses to descend, so setup failed on a valid config. The key has to be replaced with a collection node first; assigning a plain object raises the same error one level down, because the value is stored as-is rather than converted. * Serialising the document tree drops a leading byte order mark and rewrites every line with LF. Both belong to the user's file, so they are captured from the input and restored on write, which is how the JSON and TOML writers already treat them.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
OpenClaw was reported as having no rules mechanism. It has one: the workspace AGENTS.md is a bootstrap file that OpenClaw injects into the system prompt on every turn, treats as instruction context, and passes down to sub-agent sessions. It is workspace-level rather than project-level, so it is reachable from a global setup, and the rule now goes there fenced by markers like any file the user also writes to. The workspace can move, so the path follows OPENCLAW_WORKSPACE_DIR and the profile suffix before falling back to ~/.openclaw/workspace. An explicit agents.defaults.workspace in the config wins over both, but that file is JSON5 and out of reach here; landing the rule in an unused workspace is inert, unlike a misplaced server entry. The rule is only written when that AGENTS.md already exists. OpenClaw seeds the file with its own instructions on first run, and creating it first would leave the user with our section instead of those. Writing a rule and registering the server are separate concerns, so a launcher can take one without giving up its own MCP registration. Hermes still gets no rule: it reads AGENTS.md from the working directory and deliberately ignores one in $HOME, and its only global context file is SOUL.md, which is the user's agent identity rather than a place for tool routing.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Setup could carry a key or run anonymously, but not sign in, so the one path the docs present to a person at a terminal had no command behind it. `--oauth` writes the sign-in endpoint instead of a credential, and each agent starts the browser flow itself the first time it connects. Sign-in is a different server URL rather than a different header, so it replaces the credential rather than travelling beside it: an exported key is ignored under --oauth and no Authorization header is written. The two are separate endpoints, so combining --oauth with --keyless is rejected instead of silently preferring one. A URL alone is not enough for every agent. Hermes starts the flow only when the entry carries `auth: oauth`, and OpenClaw ignores a static Authorization header unless `auth: "oauth"` is set, which is also what gates its login command. Those fields live beside the agent in the registry, next to the environment syntax, and an agent without a verified sign-in shape gets no entry rather than one that reports success and then exposes no tools. No agent signs in during setup, and each one starts the flow differently, so the summary prints the step per agent rather than one footer: `/mcp` in Claude Code, `codex mcp login firecrawl`, Cursor Settings, and a browser on first use for the rest.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Four defects reported against the sign-in commit, each confirmed against the code before changing it: * Replacing an empty `mcp_servers:` key dropped an inline comment sitting on it. The comment belongs to the null value being replaced; a block map has no inline slot on its key, so it now moves to the head of the section instead of disappearing with the node it was attached to. Preserved and relocated beats silently deleted. * A marker-fenced rule section was written with LF regardless of the file it joined, so updating a CRLF AGENTS.md left mixed endings. The section now adopts the line endings of the file it is written into, which is how the JSON, TOML, and YAML writers already behave. * The OpenClaw workspace was resolved from the environment and the documented defaults alone, so a workspace moved through config took the rule to a path the launcher never reads. OpenClaw is now asked where its workspace is, since its config is JSON5 and out of reach; the previous resolution stays as the fallback for when the CLI cannot answer. * Declining rules reported OpenClaw as not supporting them, which stopped being true when it gained a rule. It reports skipped now, and unsupported is left for agents that genuinely have nowhere to put one.
installOpenClawMcp read the stored key whenever keyless was not also set, so calling it with sign-in alone produced an OAuth entry carrying an Authorization header. OpenClaw ignores a static header once auth is oauth, so the result was inert rather than harmful, but writing credential configuration into a sign-in entry is wrong either way. The CLI never reached this: setup derives keyless from the auth mode, and sign-in is not env, so the header was already dropped. The helper is exported though, and the credential helper beside it is written to be safe in isolation for the same reason, so sign-in now drops the key in the config builder where every caller passes through.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
setup mcp promises a global entry in a file we can parse, never a literal key, plus an optional rule file we own. Claude Code, Cursor, VS Code, and Codex meet that as registry data: the only difference is the file format. OpenCode is the same kind of write with its own map key and header form, so it stays. Hermes and OpenClaw were each a second product. Hermes needed a YAML writer, an owner-only create mode, an extra entry field for sign-in, and had no global rule file to own. OpenClaw needed a subprocess, a JSON5 config we cannot parse, a workspace probe, a duplicate credential builder, and a rule that could only be written if its AGENTS.md already existed. Neither shaped the product; both shaped the code around them. They stay supported and stop being written. `--hermes`, `--openclaw`, and their names on --agent print the server URL and succeed, the way an agent we install skills for but write no MCP config for already did. Skills and firecrawl launch are untouched, and the URL still works for both. Removing the two takes the whole launcher concept with them: the subprocess runner and its Windows argv escaping, the second credential path, launcher detection, the YAML writer, and the target/client split that existed only because launchers were not clients. Five agents, one contract, one writer.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…ed yaml dep Two defects reported against the scope change: * The URL-only branch returns before the writers run, and the mutual-exclusion check lived with the writers, so `--hermes --oauth --keyless` printed a sign-in URL and exited zero while `--cursor --oauth --keyless` was rejected. The check now runs at the top of installMcp, ahead of everything that reports or returns. * Removing the Hermes writer took the last import of the yaml package with it. Nothing under src/ imports it now, so it is dropped from the manifest and the lockfile rather than shipped unused. A third finding, that a test leaks FIRECRAWL_API_KEY into later tests, does not hold: beforeEach deletes that variable and afterEach restores the original, so each test starts without it. The assignment is also load-bearing where it is, since the assertion it supports is that sign-in drops a key that was present.
The sign-in mode had its own lead-in, code block, and paragraph, and the two agents setup no longer configures had a paragraph explaining why. Both restate structure the section already has: one paragraph names the supported agents, and one paragraph covers how the credential is handled. Sign-in now sits in that credential paragraph beside keyless, and the two unwritten agents sit in the sentence that already lists the supported set. Same facts, no new sections, nine fewer lines. The harness table above is scoped to skills and stays accurate as written.
A rule tells an agent to prefer firecrawl_search and firecrawl_scrape. It was written even when the server entry for that agent had failed, so an agent could be instructed to reach for tools it had no way to call. The rule now depends on the entry landing. The dependency runs one way only: a failed rule still leaves a working MCP server, which is what keeps the two writes separate. A run that configured some of the chosen agents also exited zero, so a script could not tell a partial install from a complete one. The summary already named what failed; the exit code now agrees with it. Quiet mode is unchanged, since it runs inside init and launch, which report their own outcome and continue. Also ignores .cursor/, alongside the editor directories already listed. Nothing from it was ever tracked, but it is the same kind of workspace artifact a broad `git add` swept into the repo once already.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Failing the run on a partial install swallowed the tail of `--agent all`: the throw left before the Hermes and OpenClaw lines printed, and their URL is the whole answer for those two, so the run that most needed to mention them was the one that did not. The report now runs whether or not the writers threw. The exit code is unchanged. Configuring what it can and then reporting failure is the same contract as before, since per-agent isolation is about the other agents still being configured, not about the command claiming success.
… did installMcp is shared, so making Hermes and OpenClaw URL-only reached callers this branch never edited. Three consequences, all in that seam: * `firecrawl launch hermes --install` printed the URL for an agent we do not configure and then said it was configured with Firecrawl MCP. Launch now claims MCP only when the run actually wrote it, which also covers --skip-mcp, where the same line was already claiming a write that never happened. * Naming an agent we do not configure returned from the whole run, so `--agent hermes --cursor` printed the URL and left Cursor unconfigured. The URL is now reported for whichever flag named it and the rest of the run continues; it ends early only when nothing else was named. * Detecting no agents at all is still an error, since nothing was configured and the run should not report success. It now names the server URL alongside the agent flags, so a machine we cannot detect still learns how to connect. The launch target table keeps its `mcpAgent` entries for those two agents: the call is what surfaces the URL, and dropping it would trade a false claim for silence.
init and launch render an indented block, and these lines sat flush against it.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
This reverts the scope reduction and everything built on it. The PR keeps the harnesses main already supports and changes only how MCP gets installed: Claude Code, Cursor, VS Code, Codex, OpenCode, Hermes Agent, and OpenClaw, with native writers in place of the add-mcp subprocess. Narrowing the set to the agents that share one contract is a real argument, but it is a different change: it drops harnesses, so it has to move `firecrawl launch` and the docs with it, and it should be judged on its own rather than riding along with the writer swap. It comes back as "stop auto-configuring Hermes and OpenClaw" if we want it. So the Hermes YAML writer and the OpenClaw launcher are back, `launch` configures both exactly as it does on main, and the URL-only path invented here is gone rather than left as a second way an agent can be supported. Kept from the reverted range, since none of it depends on the smaller set: the auth-mode check ahead of every branch, rules not being written for an agent whose server entry failed, a partial run failing the command, and .cursor/ in .gitignore. The launcher isolation test now asserts both halves of that last one: OpenClaw failing is reported, and Cursor beside it is still configured.
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Those three branches passed `yes: true` into installMcpClients to skip the picker, but naming the targets already does that: the picker only runs when no targets were selected, and each of these arrives with a list. What `yes` still reached was the rules prompt, so `--agent cursor` asked whether to install rules and `--agent all` silently decided no. Dropping the flag leaves the picker skipped as before and lets the question be asked. Nothing else moves: without a TTY the run is still non-interactive, `-y` still declines rules, and `--rules` still forces them.
…her gaps The Hermes/OpenClaw revert brought back unguarded launcher rule writes, PATH false positives, a Windows workspace probe that swallowed .cmd failures, and YAML setIn crashes on non-mapping mcp_servers.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The oauth report prefixed every step with "Sign in" and then repeated the verb inside the step itself, so VS Code read "Sign in sign in from the MCP view in VS Code". Checking each agent against its own docs showed the duplication was hiding worse problems: VS Code has no MCP view and runs client registration itself, Cursor surfaces a needs-login control of its own, OpenCode prompts on first use, and Claude Code shows a startup notice for any server that answers 401, which the sign-in endpoint does. Those four now print nothing. The three that do need a command keep one, and the strings are corrected: Codex leads with its own command but names the Authenticate action, since the desktop app and the IDE extension share the config file this writes and only the CLI needs the command. Hermes reloads config on a 30s timer that cannot outlast an interactive flow, so it names the login command and says to run it from a new terminal rather than claiming the browser opens by itself. The footer no longer promises that every agent prompts you.
Hermes has no global rules file to own, so every run ended its block with "Rules not supported by this agent" — a line about something the user never asked for and cannot act on. The skipped case still prints, because that one means a rule was requested and did not land.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The integrations checkbox named three editors, but MCP setup writes to every agent it detects, so the list both undersold it and went stale as agents were added. It now reads like the skills entry above it and names no agents. printNextSteps also offered "Add MCP: firecrawl setup mcp" unconditionally, including right below its own "✓ MCP server installed". stepIntegrations now reports whether the install succeeded, and the line prints only for someone who does not have it: skipping the integration or a failed install both keep it.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Quiet mode fails only when nothing lands, so a run where one agent failed resolved normally and init printed "✓ MCP server installed" directly under its own ✗ line, then hid the "Add MCP" next step that would have fixed it. installMcp now returns whether every targeted agent was configured, and init claims success only on that; a partial run points at "firecrawl setup mcp" and keeps the next step. The OpenClaw workspace lookup also expanded any leading tilde against home, so a `~other/ws` workspace, which names another account to a shell, wrote the rule into $HOME/other/ws instead. Expansion is now limited to `~` and a `~/` prefix, and reads the resolved home from the context the writers already carry rather than calling os.homedir() again.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Hermes has no global rule file, so --rules was going silent: clients only reach unsupported after rules were requested, and the dropped line hid that.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/setup.ts">
<violation number="1" location="src/commands/setup.ts:849">
P3: The new `unsupported` branch prints `Rules not supported by this agent` unconditionally, but launchers reach `unsupported` even when rules were never requested. In `setupMcpLauncher`, `ruleStatus` starts as `'unsupported'` and stays that way whenever `MCP_LAUNCHER_RULES[id]` is undefined (`if (!rule) return result;`), regardless of the `rules` flag. The comment's rationale (`setupMcpClient returns skipped whenever rules is false`) only holds for `setupMcpClient`; `ruleLine` is also called for launchers. So a plain MCP-only run like `setup mcp -y` with Hermes (which has no launcher rule) now prints `Rules not supported by this agent` even though nothing was requested — the same `-y` noise this change was meant to avoid. Restrict the message to the case where rules were actually requested, or have the launcher return `skipped` when `rules` is false so the unsupported message is accurate.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Fix all with cubic | Re-trigger cubic
| // Clients only reach this when rules were requested: setupMcpClient | ||
| // returns `skipped` whenever rules is false. Hermes has no global rule | ||
| // file, so `--rules` has to say so rather than going silent. | ||
| return ` Rules ${dim}not supported by this agent${reset}`; |
There was a problem hiding this comment.
P3: The new unsupported branch prints Rules not supported by this agent unconditionally, but launchers reach unsupported even when rules were never requested. In setupMcpLauncher, ruleStatus starts as 'unsupported' and stays that way whenever MCP_LAUNCHER_RULES[id] is undefined (if (!rule) return result;), regardless of the rules flag. The comment's rationale (setupMcpClient returns skipped whenever rules is false) only holds for setupMcpClient; ruleLine is also called for launchers. So a plain MCP-only run like setup mcp -y with Hermes (which has no launcher rule) now prints Rules not supported by this agent even though nothing was requested — the same -y noise this change was meant to avoid. Restrict the message to the case where rules were actually requested, or have the launcher return skipped when rules is false so the unsupported message is accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/setup.ts, line 849:
<comment>The new `unsupported` branch prints `Rules not supported by this agent` unconditionally, but launchers reach `unsupported` even when rules were never requested. In `setupMcpLauncher`, `ruleStatus` starts as `'unsupported'` and stays that way whenever `MCP_LAUNCHER_RULES[id]` is undefined (`if (!rule) return result;`), regardless of the `rules` flag. The comment's rationale (`setupMcpClient returns skipped whenever rules is false`) only holds for `setupMcpClient`; `ruleLine` is also called for launchers. So a plain MCP-only run like `setup mcp -y` with Hermes (which has no launcher rule) now prints `Rules not supported by this agent` even though nothing was requested — the same `-y` noise this change was meant to avoid. Restrict the message to the case where rules were actually requested, or have the launcher return `skipped` when `rules` is false so the unsupported message is accurate.</comment>
<file context>
@@ -843,11 +843,16 @@ function ruleLine(
+ // Clients only reach this when rules were requested: setupMcpClient
+ // returns `skipped` whenever rules is false. Hermes has no global rule
+ // file, so `--rules` has to say so rather than going silent.
+ return ` Rules ${dim}not supported by this agent${reset}`;
case 'failed':
return ` ${red}Rules failed${reset} ${result.ruleDetail}`;
</file context>
Parse Codex TOML by AST so quoted and BOM-prefixed tables are updated in place, honor CODEX_HOME and HERMES_HOME, write VS Code rules to the documented instructions path, and reject empty install-mode runs.
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Do not fail the default setup bundle after skills when no agents are installed, and treat a recovered JSONC tree as registered when it still contains Firecrawl.
upsertTomlServer replaced everything between the first Firecrawl table and the last, so an unrelated [mcp_servers.other] sitting between the entry and its env sub-table was deleted along with them. Each table is now removed on its own, in reverse order so the earlier offsets stay valid in the mutated string, and the replacement goes in where the first one started. The first table keeps its terminating newline. The replacement block is written without one, so a config whose next table follows immediately, which is the documented stdio layout of [mcp_servers.firecrawl] above [mcp_servers.firecrawl.env], came back as `url = "..."[mcp_servers.other]` and failed to reparse. The user saw "could not parse existing config" about a file that was valid TOML. runClientCommand moves to its own module. Doctor's OpenClaw probe called execFileSync directly, which cannot launch a .cmd shim, so on Windows it reported the server unregistered for the exact reason the wrapper exists. OpenClaw also counts as installed when the launcher is on PATH rather than only when ~/.openclaw exists, which is what setup already detects.
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/utils/run-client-command.ts">
<violation number="1" location="src/utils/run-client-command.ts:11">
P2: When a `.cmd` launcher receives an argument containing `%NAME%`, `escapeCmdArg` leaves the percent signs unescaped, so `cmd.exe` expands the value before the launcher sees it. Escape percent expansion using cmd-specific rules and add coverage for paired-percent arguments so this helper preserves argv values.</violation>
<violation number="2" location="src/utils/run-client-command.ts:59">
P2: When an earlier PATH entry contains a directory named `<command><PATHEXT>`, `resolveWindowsCommand` selects it because `existsSync` accepts directories, then launcher setup fails instead of continuing to the real executable. Require a regular file before returning a PATH candidate.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| for (const directory of pathEntries) { | ||
| for (const candidate of candidates) { | ||
| const resolved = directory ? path.join(directory, candidate) : candidate; | ||
| if (existsSync(resolved)) return resolved; |
There was a problem hiding this comment.
P2: When an earlier PATH entry contains a directory named <command><PATHEXT>, resolveWindowsCommand selects it because existsSync accepts directories, then launcher setup fails instead of continuing to the real executable. Require a regular file before returning a PATH candidate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/run-client-command.ts, line 59:
<comment>When an earlier PATH entry contains a directory named `<command><PATHEXT>`, `resolveWindowsCommand` selects it because `existsSync` accepts directories, then launcher setup fails instead of continuing to the real executable. Require a regular file before returning a PATH candidate.</comment>
<file context>
@@ -0,0 +1,94 @@
+ for (const directory of pathEntries) {
+ for (const candidate of candidates) {
+ const resolved = directory ? path.join(directory, candidate) : candidate;
+ if (existsSync(resolved)) return resolved;
+ }
+ }
</file context>
| import { existsSync } from 'fs'; | ||
| import path from 'path'; | ||
|
|
||
| const CMD_META_CHARS = /([()%!^"<>&|])/g; |
There was a problem hiding this comment.
P2: When a .cmd launcher receives an argument containing %NAME%, escapeCmdArg leaves the percent signs unescaped, so cmd.exe expands the value before the launcher sees it. Escape percent expansion using cmd-specific rules and add coverage for paired-percent arguments so this helper preserves argv values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/run-client-command.ts, line 11:
<comment>When a `.cmd` launcher receives an argument containing `%NAME%`, `escapeCmdArg` leaves the percent signs unescaped, so `cmd.exe` expands the value before the launcher sees it. Escape percent expansion using cmd-specific rules and add coverage for paired-percent arguments so this helper preserves argv values.</comment>
<file context>
@@ -0,0 +1,94 @@
+import { existsSync } from 'fs';
+import path from 'path';
+
+const CMD_META_CHARS = /([()%!^"<>&|])/g;
+
+function rejectCommandControlCharacters(value: string, label: string): void {
</file context>
init passed `--agent all` whenever FIRECRAWL_API_KEY was exported. That was a workaround for a guard this branch removed, back when environment-backed setup refused to run without a named agent, and `all` now means every supported agent whether or not it is installed. On a machine with only Claude Code it wrote six config files, including ~/.cursor/mcp.json and VS Code's, then failed on OpenClaw and ended the run pointing at "firecrawl setup mcp". With no agent named, detection runs instead and resolves the exported key on its own, so the same machine gets one file and a clean finish. The rule fence now pairs the last two markers rather than the first two. A file carrying an odd marker, from a half-written run or a hand edit, paired that stray one with our opening marker, so the next run replaced the span between them and took the user's own text with it. Also: `openclaw config get` gets the 8s timeout doctor's probe already has, since its stderr is discarded and a wedged launcher would otherwise hang setup with nothing on screen; a launcher result records the mode the run configured instead of folding oauth in with keyless; and the `--agent` help no longer advertises the environment-backed requirement that is gone.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/utils/mcp-install.ts">
<violation number="1" location="src/utils/mcp-install.ts:328">
P2: When a user-authored `` marker appears after the managed section, `appendRuleSection` treats the last two markers as its fence and deletes the text between them on rerun. Identify the owned section without assuming the last two markers, or avoid replacement when ownership is ambiguous.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // from a half-written run or a hand edit, would otherwise pair that stray | ||
| // one with our opening marker and delete everything the user wrote between | ||
| // them. Our own section is always the final pair. | ||
| const close = existing.lastIndexOf(RULE_MARKER); |
There was a problem hiding this comment.
P2: When a user-authored <!-- firecrawl --> marker appears after the managed section, appendRuleSection treats the last two markers as its fence and deletes the text between them on rerun. Identify the owned section without assuming the last two markers, or avoid replacement when ownership is ambiguous.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/mcp-install.ts, line 328:
<comment>When a user-authored `<!-- firecrawl -->` marker appears after the managed section, `appendRuleSection` treats the last two markers as its fence and deletes the text between them on rerun. Identify the owned section without assuming the last two markers, or avoid replacement when ownership is ambiguous.</comment>
<file context>
@@ -324,15 +320,21 @@ export async function appendRuleSection(
+ // from a half-written run or a hand edit, would otherwise pair that stray
+ // one with our opening marker and delete everything the user wrote between
+ // them. Our own section is always the final pair.
+ const close = existing.lastIndexOf(RULE_MARKER);
+ const open = close > 0 ? existing.lastIndexOf(RULE_MARKER, close - 1) : -1;
+ const opensWithNewline = /^\r?\n/.test(
</file context>
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
setup mcpshelled out to a third-party installer, putting a package we don't control on the critical path of a first-run command and capping which agents we support. It also only ever installed the server; nothing told the agent to actually use Firecrawl for web work.Summary
setup mcpnow configures agents directly: detects what's installed, lists those in a picker (already selected), then offers to add rules that make agents prefer Firecrawl for web search and scraping.--rules/--no-rulesfor scripts.-ystays MCP-only, so automation never rewrites instruction files. If nothing is detected, setup asks for an agent flag (--cursor,--agent all) instead of opening an empty picker.-gis still accepted for existing scripts, hidden from help, and reported as deprecated when used.-gis unchanged oninitandlaunch.--agent windsurf, for example) finishes the run, skips the MCP step, and prints the server URL. A name nothing supports is still rejected.FIRECRAWL_API_KEYis exported where the agent will run, each agent gets a reference to that variable in the syntax it expands. Otherwise setup stays keyless and says so.--hermes/--openclaw(and the matching--agentnames) use that same keyless fallback; they no longer abort on a stored-only key.--agent allstill fail-closes on a stored-only key, because that path reaches the launcher CLIs.JSON.parserejects outright. A leading byte order mark is preserved rather than treated as a parse failure. TOML tables are replaced cleanly, including stale sub-tables from an earlier stdio entry, and CRLF files keep their line endings instead of gaining a duplicate table. Reruns are byte-identical.initandlaunchcannot report success over an unwritten config.Test Plan
pnpm test,type-check,format:check,buildsetup mcp --claude --cursorand-ywrite global config only;--projectis unknown--agent hermesand--hermesboth write keyless YAML;--agent openclawand--openclawboth write keyless OpenClaw config; no literal key on diskFIRECRAWL_API_KEY: each agent gets its native env reference, not the literal