feat(cli): configure MCP natively across coding agents - #187
Open
Max17190 wants to merge 41 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
Max17190
force-pushed
the
feat/native-mcp-setup
branch
from
August 12, 2026 21:55
cc7e59b to
9162131
Compare
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
Max17190
force-pushed
the
feat/native-mcp-setup
branch
from
August 18, 2026 22:18
79f2aa6 to
b7c9ab8
Compare
Contributor
Author
|
@cubic-dev-ai review this PR |
@Max17190 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 8 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.
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.
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.
…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.
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.
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 a third-party installer 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.
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.
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.
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.
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.
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.
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.
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.
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.
…e writing setup defaults matched web_search by line and rewrote whatever it found. A user who had deliberately set web_search = "enabled" got it flipped to "disabled", and the inline comment explaining why went with it. The already-disabled check compared the whole line as a string, so web_search="disabled" without spaces was rewritten too and reported as a change. A test asserted the overwrite, so it was locked in rather than accidental. web_search is now read from the TOML AST that this branch already parses for Codex MCP config. A value the user set is reported and left alone; only an absent key is written, and only the value this command writes is removed on undo. A config.toml that does not parse is skipped rather than rewritten. Claude settings are patched through the same JSONC editor used for MCP config, so comments survive. Previously they were stripped to parse and lost on write. Both writers can run as a dry run, so an interactive run now prints the exact line each agent will receive and asks before touching anything. --yes and non-interactive runs are unchanged. The closing line no longer claims an agent was configured when its existing value was kept.
…rules setup mcp wrote instruction files telling each agent to prefer Firecrawl for web work. A rule is only advice: the agent can still call its own web search, and it cost a rule spec per agent, a marker-fenced writer for shared files, a launcher workspace probe, and a five-state reporting surface. Registering the server is now all setup mcp writes. In its place the command offers a second step: hand its native web search and fetch over to Firecrawl. Claude Code and Codex each document a switch for that, so those are the two agents offered, and only when their server entry landed in the same run. An agent left without a working Firecrawl server never has its own tools taken away. Turning off a built-in tool is a real behavior change, so it is confirmed: the exact line for each config is printed before anything is written, a value already set is reported and kept rather than replaced, and declining writes nothing. -y stays server-only; automation opts in with --defaults and out with --no-defaults. setup defaults still runs the same step on its own. Removes the rule spec, the rule writers, the marker fence, the launcher workspace lookup, and the dead local left over from resolving auth in installMcpClients.
When one selected agent failed and Claude Code or Codex succeeded, reporting threw before the handover ran, so --defaults never reached the agent that was configured. The step is already scoped to agents whose entry landed, so the throw was acting as a safety gate it did not need to be. Reporting now only reports. The aggregate failure moves to its own step that runs after the handover, keeping both existing outcomes: quiet callers still fail only when nothing landed, and an interactive run still fails when any requested agent did not.
Four defects in the defaults writer, all reproduced before the fix.
The Codex insert anchored on the first line of the last root key rather than the
end of its value, so a multi-line root value had web_search spliced into the
middle of it. An inline array left the file unparseable while the run reported
success; a triple-quoted string silently corrupted the user's value and never
set web_search. The offset now comes from the end of the value, and the edited
document is re-parsed before it is written, matching how the MCP TOML writer
already guards itself.
Claude settings guarded the read of `permissions` against a non-object but not
the write, so `{"permissions": null}` parsed fine, previewed fine, was confirmed
by the user, and then threw out of jsonc-parser. A permissions value of the
wrong shape now has the whole key replaced, the same way the MCP JSON writer
handles a malformed servers map.
Both files ignored CODEX_HOME and CLAUDE_CONFIG_DIR, which this branch honours
everywhere else, so on a relocated config tree the server entry and the switch
landed in different places and the switch went to a file the agent never reads.
The step is an offer on top of the work the command was asked to do, so an
unreadable file or a permissions error no longer fails a run whose MCP write
already succeeded and was reported.
Also drops an em dash from an init picker label.
Max17190
force-pushed
the
feat/native-mcp-setup
branch
from
August 31, 2026 21:26
0ca3984 to
755a8a8
Compare
There was a problem hiding this comment.
4 issues found across 18 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/web-defaults.ts">
<violation number="1" location="src/utils/web-defaults.ts:697">
P1: When Claude settings contain comments or trailing commas, `JSON.parse` rejects them before the JSONC edit runs, so setup skips the handover instead of preserving the file and adding the Firecrawl denies. Parse with the JSONC parser while retaining the duplicate-key and parse-error checks.</violation>
</file>
<file name="src/utils/mcp-clients.ts">
<violation number="1" location="src/utils/mcp-clients.ts:130">
P2: When Hermes has an `active_profile` marker for a missing profile, `hermesHome` throws and can abort `firecrawl doctor` for every agent. Keep profile-resolution failures isolated to Hermes detection, or make the doctor detection path handle this error without rejecting the aggregate scan.</violation>
</file>
<file name="src/commands/setup.ts">
<violation number="1" location="src/commands/setup.ts:1397">
P1: When Claude or Codex already has the exact entry, this filter still performs the web handover on reruns. Restrict handover targets to `configured` and `reconfigured` results so native tools change only after a write in this run.</violation>
</file>
<file name="src/utils/atomic-file.ts">
<violation number="1" location="src/utils/atomic-file.ts:252">
P2: When another writer changes the file after the final snapshot but before this rename, the unconditional rename overwrites that writer's update despite the expected-content checks. Serialize writers with a lock or use a conditional commit mechanism around validation and rename.</violation>
</file>
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
| let parsed: unknown; | ||
| try { | ||
| config = JSON.parse(removeJsonComments(existing)); | ||
| parsed = JSON.parse(raw); |
There was a problem hiding this comment.
P1: When Claude settings contain comments or trailing commas, JSON.parse rejects them before the JSONC edit runs, so setup skips the handover instead of preserving the file and adding the Firecrawl denies. Parse with the JSONC parser while retaining the duplicate-key and parse-error checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/web-defaults.ts, line 697:
<comment>When Claude settings contain comments or trailing commas, `JSON.parse` rejects them before the JSONC edit runs, so setup skips the handover instead of preserving the file and adding the Firecrawl denies. Parse with the JSONC parser while retaining the duplicate-key and parse-error checks.</comment>
<file context>
@@ -35,168 +32,835 @@ export interface WebDefaultsOptions {
+ }
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(raw);
+ } catch {
+ return skippedClaude(filePath, 'settings.json is not valid JSON');
</file context>
| ): result is McpClientResult & { | ||
| id: Extract<McpClientId, 'claude' | 'codex'>; | ||
| } => | ||
| result.mcpStatus !== 'failed' && |
There was a problem hiding this comment.
P1: When Claude or Codex already has the exact entry, this filter still performs the web handover on reruns. Restrict handover targets to configured and reconfigured results so native tools change only after a write in this run.
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 1397:
<comment>When Claude or Codex already has the exact entry, this filter still performs the web handover on reruns. Restrict handover targets to `configured` and `reconfigured` results so native tools change only after a write in this run.</comment>
<file context>
@@ -684,401 +790,949 @@ export async function installSkillsForAgent(
+ ): result is McpClientResult & {
+ id: Extract<McpClientId, 'claude' | 'codex'>;
+ } =>
+ result.mcpStatus !== 'failed' &&
+ (result.id === 'claude' || result.id === 'codex')
+ )
</file context>
Suggested change
| result.mcpStatus !== 'failed' && | |
| (result.mcpStatus === 'configured' || | |
| result.mcpStatus === 'reconfigured') && |
| } catch { | ||
| // Report one deterministic error below. | ||
| } | ||
| throw new Error( |
There was a problem hiding this comment.
P2: When Hermes has an active_profile marker for a missing profile, hermesHome throws and can abort firecrawl doctor for every agent. Keep profile-resolution failures isolated to Hermes detection, or make the doctor detection path handle this error without rejecting the aggregate scan.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/mcp-clients.ts, line 130:
<comment>When Hermes has an `active_profile` marker for a missing profile, `hermesHome` throws and can abort `firecrawl doctor` for every agent. Keep profile-resolution failures isolated to Hermes detection, or make the doctor detection path handle this error without rejecting the aggregate scan.</comment>
<file context>
@@ -83,17 +84,52 @@ export function createMcpContext(
+ } catch {
+ // Report one deterministic error below.
+ }
+ throw new Error(
+ `Hermes active profile "${active}" does not exist at ${profileHome}`
+ );
</file context>
| const committedIdentity: AtomicPathIdentity = existing?.isSymbolicLink() | ||
| ? { link: statsIdentity(existing), target: preparedIdentity } | ||
| : { link: preparedIdentity, target: preparedIdentity }; | ||
| await fs.rename(temporary, target); |
There was a problem hiding this comment.
P2: When another writer changes the file after the final snapshot but before this rename, the unconditional rename overwrites that writer's update despite the expected-content checks. Serialize writers with a lock or use a conditional commit mechanism around validation and rename.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/atomic-file.ts, line 252:
<comment>When another writer changes the file after the final snapshot but before this rename, the unconditional rename overwrites that writer's update despite the expected-content checks. Serialize writers with a lock or use a conditional commit mechanism around validation and rename.</comment>
<file context>
@@ -0,0 +1,312 @@
+ const committedIdentity: AtomicPathIdentity = existing?.isSymbolicLink()
+ ? { link: statsIdentity(existing), target: preparedIdentity }
+ : { link: preparedIdentity, target: preparedIdentity };
+ await fs.rename(temporary, target);
+ await syncDirectory(path.dirname(target));
+ return committedIdentity;
</file context>
There was a problem hiding this comment.
1 issue found across 9 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/__tests__/commands/doctor.test.ts">
<violation number="1" location="src/__tests__/commands/doctor.test.ts:382">
P2: These new Hermes tests place the profile under tmpHome/.hermes and depend on the os.homedir() mock to reach it, but hermesRoot() on Windows resolves from LOCALAPPDATA (not homedir), which the beforeEach never clears. On a Windows test run hermes.installed becomes false and the assertions fail. Set process.env.HERMES_HOME to the temp root instead (the suite's existing convention in 'follows HERMES_HOME'), and clear LOCALAPPDATA in the runChecks test backing it up.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| ['an invalid marker', '../escape\n'], | ||
| ['a missing named profile', 'work\n'], | ||
| ])('isolates Hermes detection for %s', async (_case, marker) => { | ||
| const hermesRoot = path.join(tmpHome, '.hermes'); |
There was a problem hiding this comment.
P2: These new Hermes tests place the profile under tmpHome/.hermes and depend on the os.homedir() mock to reach it, but hermesRoot() on Windows resolves from LOCALAPPDATA (not homedir), which the beforeEach never clears. On a Windows test run hermes.installed becomes false and the assertions fail. Set process.env.HERMES_HOME to the temp root instead (the suite's existing convention in 'follows HERMES_HOME'), and clear LOCALAPPDATA in the runChecks test backing it up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/__tests__/commands/doctor.test.ts, line 382:
<comment>These new Hermes tests place the profile under tmpHome/.hermes and depend on the os.homedir() mock to reach it, but hermesRoot() on Windows resolves from LOCALAPPDATA (not homedir), which the beforeEach never clears. On a Windows test run hermes.installed becomes false and the assertions fail. Set process.env.HERMES_HOME to the temp root instead (the suite's existing convention in 'follows HERMES_HOME'), and clear LOCALAPPDATA in the runChecks test backing it up.</comment>
<file context>
@@ -375,6 +375,31 @@ describe('detectAgents', () => {
+ ['an invalid marker', '../escape\n'],
+ ['a missing named profile', 'work\n'],
+ ])('isolates Hermes detection for %s', async (_case, marker) => {
+ const hermesRoot = path.join(tmpHome, '.hermes');
+ fs.mkdirSync(hermesRoot, { recursive: true });
+ fs.writeFileSync(path.join(hermesRoot, 'active_profile'), marker);
</file context>
Max17190
force-pushed
the
feat/native-mcp-setup
branch
from
September 1, 2026 04:39
4672552 to
755a8a8
Compare
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 mcpshould register Firecrawl directly and predictably in supported agent harnesses without persisting API keys. When users explicitly choose Firecrawl as their default web provider, setup should change only the documented settings and preserve unrelated configuration.Summary
Test Plan
pnpm test(538 tests across 25 files)pnpm type-checkpnpm buildpnpm format:checkgit diff --check