Skip to content

feat(dometrain): split the MCP server into dometrain-mcp - #5213

Merged
kyle-sexton merged 3 commits into
mainfrom
feat/dometrain-split-mcp
Sep 29, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
feat/dometrain-split-mcp

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Refs #5209

Summary

Implements option 4 of #5209. dometrain becomes skills only (grounding, setup, sync) at 0.5.0. The bundled HTTP server (https://mcp.dometrain.com/mcp) and the dometrain_api_key option move to a new plugin, dometrain-mcp 0.1.0 (optional, sensitive key; defaultEnabled: false).

  • A user with their own user-scope dometrain server installs dometrain only: no plugin server at the same endpoint, so no duplicate-server warning.
  • A user with no server installs dometrain and dometrain-mcp and keeps the zero-setup path.
  • grounding and setup accept mcp__plugin_dometrain-mcp_dometrain__* (plugin) and mcp__dometrain__* (user-scope). With neither present, setup names both supported setups. The prefix keeps the hyphen, as the live tool list shows for dotnet-msbuild (mcp__plugin_dotnet-msbuild_binlog__*).
  • The env-var and vault-exec override recipes moved to the dometrain-mcp README and now mean "skip dometrain-mcp".
  • Registered in marketplace.json, docs/catalog.md (generated), and .claude/settings.json enabledPlugins (false, matching animation). docs/cloud-sessions.md names dometrain-mcp as the plugin holding the key.

Migration impact for existing dometrain users

  • Bundled-server users: install dometrain-mcp and enter the key again there. Claude Code does not carry pluginConfigs between plugins. Until then the skills report no server.
  • User-scope server users: no change; updating removes the plugin server, which was the source of the warning.
  • Permission rules on mcp__plugin_dometrain_dometrain__* need mcp__plugin_dometrain-mcp_dometrain__*.
  • The dometrain changelog and README ("Upgrading from 0.4.x") say this.

Verification

Run in the worktree, all passing:

  • scripts/validate-plugins.sh (all manifests plus strict catalog)
  • scripts/validate-plugin-contracts.mjs (58 setup skills checked)
  • scripts/check-plugin-catalog-enablement.sh --check (failed before the enabledPlugins key, passes after)
  • scripts/check-changelog-parity.sh --check, --check-bump origin/main, --check-order
  • scripts/check-plugin-manifest-presence.sh --check
  • scripts/generate-catalog.mjs --check, scripts/sync-plugin-options-docs.py --check, scripts/generate-cheatsheet.mjs (unchanged)
  • scripts/check-skill-count-claims.sh (0 mismatched), scripts/check-purged-em-dashes.sh (none)
  • markdownlint-cli2 on the touched markdown (0 issues)
  • skill-quality check-skill.sh plugins/dometrain/skills (3 pass, 0 errors; warnings only) and check-evals-quality.sh on the setup evals

Not verified

Conflict note

Draft #5210 edits the old dometrain README and will conflict with this branch. Its branch is untouched.

🤖 Generated with Claude Code

https://claude.ai/code/session_01H4pw3AKPP9sNePN5qwf9jL

dometrain becomes skills only (0.5.0, breaking): the bundled .mcp.json and
the dometrain_api_key option move to the new dometrain-mcp plugin (0.1.0).
A user with their own user-scope dometrain server installs dometrain alone
and gets no duplicate-server warning; a user with no server installs both.

Grounding and setup accept mcp__plugin_dometrain-mcp_dometrain__* and
mcp__dometrain__*, and setup names both supported setups when neither is
present. The override recipes move to the dometrain-mcp README and now mean
"skip dometrain-mcp". Registers the plugin in the marketplace, catalog, and
enabledPlugins.

Refs #5209

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H4pw3AKPP9sNePN5qwf9jL
@github-actions

Copy link
Copy Markdown
Contributor

PR body contract — issue linkage

This PR body does not yet satisfy the issue-linkage contract:

  • Missing a "## Fix" section. State the concrete change and how it addresses the problem.
  • Missing a "## Related" section. List related PRs, ADRs, or decision-log entries this PR does not close.
  • Missing a native closing keyword (Closes/Fixes/Resolves #N). If this PR references an issue it must not close, put "Refs: #N" (or "Relates to: #N") on its own line. If it relates to no GitHub issue at all, state "No linked issue" (or "No related issue:") in the body instead.

Edit the body and this comment updates itself on the next run.

@kyle-sexton
kyle-sexton marked this pull request as ready for review September 29, 2026 01:48
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T01:51:35.405135Z 4f87520 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 26s —— View job


Claude finished the code review

  • Gather context (PR body, diff, changed files)
  • Invoke /review:code-review and follow it exactly
  • Post inline findings (none met the bar — see below)
  • Post summary comment

Summary

Reviewed the full diff (gh pr diff 5213, 15 files, +471/-367) against the /review:code-review lane's criteria: correctness, CLAUDE.md/AGENTS.md alignment, error handling, maintainability, scoped to changed files only.

This is a mechanical plugin split (dometrain → skills-only, new dometrain-mcp for the bundled server) with no executable/runtime code — only JSON manifests, Markdown docs, and SKILL.md/evals content. I traced the moving pieces end to end:

  • plugins/dometrain-mcp/.mcp.json (renamed, unchanged) still declares its server as dometrain, which combined with the plugin name dometrain-mcp correctly produces the mcp__plugin_dometrain-mcp_dometrain__* tool prefix referenced throughout the new README, grounding/SKILL.md, and setup/SKILL.md.
  • Cross-file anchor links resolve correctly: setup/SKILL.md → ../../README.md#getting-the-mcp-server and #adding-your-own-server, and dometrain/README.md → ../dometrain-mcp/README.md#using-your-own-server-instead all match actual headings in the target files.
  • .claude-plugin/marketplace.json's new dometrain-mcp entry and .claude/settings.json's new enabledPlugins["dometrain-mcp@melodic-software"] = false are consistent with the fleet-enablement convention documented in docs/cloud-sessions.md (plugins that ship defaultEnabled: false only need an explicit settings.json key when the fleet list would otherwise enable them by default — same treatment as miro). Alphabetical ordering in enabledPlugins is preserved.
  • plugins/dometrain/skills/setup/evals/evals.json expectations were updated in step with the corresponding setup/SKILL.md task-list changes (both-prefix checking, duplicate-server warning, two-setup naming).
  • Version bumps (dometrain 0.4.1→0.5.0, dometrain-mcp new at 0.1.0) and both CHANGELOG.md entries line up with the described breaking change and migration guidance.

I did not find a correctness or maintainability defect that clears this lane's high-signal bar (a linter/typechecker-catchable issue, a pre-existing issue on untouched lines, or a style nit don't count, and none of what I found rises above that). No inline comments were posted.

Not independently re-run here (author-claimed in the PR body, not verified by this review): the shell script suite (validate-plugins.sh, check-plugin-catalog-enablement.sh, check-changelog-parity.sh, etc.), generate-catalog.mjs --check, and the skill-quality/evals checks.

Branch: feat/dometrain-split-mcp

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 31s —— View job


Security review (/review:security-review)

  • Gather context (PR diff via gh pr diff 5213, changed files)
  • Skip-gate check — has security-relevant surface (sensitive API key moves plugins), so full review performed
  • Review new dometrain-mcp plugin (API key handling, manifest, MCP config)
  • Review dometrain plugin changes (config, skills)
  • Review settings/catalog/marketplace changes
  • Post findings

No security issues found.

This PR is a config/documentation restructuring: it moves the dometrain_api_key userConfig
declaration and the bundled .mcp.json HTTP server from plugins/dometrain to a new
plugins/dometrain-mcp plugin, with matching manifest/catalog/settings updates. Checked
specifically:

  • Credential handling. plugins/dometrain-mcp/.claude-plugin/plugin.json keeps
    "sensitive": true on dometrain_api_key, and the .mcp.json (pure rename, 0 content diff)
    still references ${user_config.dometrain_api_key} in the Authorization header — the
    key never appears in plaintext, and continues to route through Claude Code's own masked
    userConfig/secure-credential-storage mechanism rather than anything this repo implements.
    No hardcoded keys or tokens found in the diff (plugin.json, README.md, CHANGELOG.md).
  • Default enablement. The new plugin ships defaultEnabled: false in both
    plugin.json and .claude-plugin/marketplace.json, and .claude/settings.json adds
    "dometrain-mcp@melodic-software": false — consistent with the existing opt-in pattern for
    other plugins holding sensitive remote-service credentials (e.g. miro).
  • Instruction-surface check. plugins/dometrain/skills/setup/SKILL.md drops the
    dometrain-specific boundary "Do not read, echo, log, copy, or persist the API key" in favor
    of a broadened "any API key" version, and drops the Keychain/credentials-file "never read or
    reveal" line from its Output section. Neither is a gap in practice: dometrain no longer owns
    any credential (its userConfig was removed entirely), and dometrain-mcp ships no skill at
    all, so there is no model-facing surface in either plugin that could act on the key — the
    actual control (native masked prompt + OS keychain storage) is unchanged and untouched by this
    diff.
  • No injection or trust-boundary changes. No new command execution, no changes to how the
    headersHelper/vault-exec opt-in recipe works (only relocated between READMEs), no
    permission or Actions workflow changes.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

"type": "http",
"url": "https://mcp.dometrain.com/mcp",
"headers": { "Authorization": "Bearer ${user_config.dometrain_api_key}" }

P2 Badge Reassign the remote-server security review

Moving the authenticated remote server into this new plugin leaves the security inventory and acceptance record attached to the wrong artifact: docs/migration-playbook.md:894-904 still says dometrain is the only remote-server plugin, while its record at lines 1047-1076 evaluates dometrain and its former userConfig. Consequently, a trust audit of dometrain-mcp finds no record covering its third-party data egress and credential surface; update or repeat that record under the new plugin identity.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/dometrain/skills/grounding/SKILL.md Outdated
The grounding and setup skills hard-coded the plugin tool prefix, a volatile
harness format. They now match the dometrain server segment and tool name in
the runtime inventory. The remaining literal prefix lives in the dometrain-mcp
README with a basis, as-of date and recheck trigger.

Refs #5209

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H4pw3AKPP9sNePN5qwf9jL
@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 39s —— View job


Security review (/review:security-review)

  • Gather context (PR body, diff via gh pr diff 5213, changed files)
  • Skip-gate check — sensitive API key and remote MCP server surface moves between plugins, so full review performed
  • Review dometrain-mcp (new plugin: manifest, .mcp.json, README)
  • Review dometrain (skills, manifest, README — server/credential removed)
  • Review settings/catalog/marketplace/docs changes
  • Post findings

No security issues found.

Reviewed all 15 files changed in this PR (head 428bb9a) against this lane's criteria:
logic flaws, authz/access gaps, injection, and unsafe secret handling. This is a plugin split
(bundled MCP server + dometrain_api_key move from dometrain to a new dometrain-mcp
plugin) with no executable code — manifests, Markdown, and SKILL.md/evals content only.

  • Credential handling unchanged. plugins/dometrain-mcp/.mcp.json is a pure rename (0 content diff): the Bearer header still resolves through ${user_config.dometrain_api_key}, never hardcoded. plugins/dometrain-mcp/.claude-plugin/plugin.json keeps "sensitive": true and defaultEnabled: false, matching the existing opt-in pattern for other credentialed remote-service plugins (e.g. miro). .claude/settings.json adds the matching "dometrain-mcp@melodic-software": false fleet-enablement key.
  • Tool-matching fix verified. The prior review round (Codex, commit 4f87520) flagged a hardcoded mcp__plugin_dometrain-mcp_dometrain__ prefix in the skill bodies as a volatile-harness-behavior risk. The follow-up commit (428bb9a, "match tools by server segment, not a fixed prefix") resolves this: grounding/SKILL.md and setup/SKILL.md now match on the dometrain server segment plus tool name rather than a literal full prefix; no hardcoded mcp__plugin_dometrain... string remains in either skill body (confirmed by grep — the only remaining literal prefix is in dometrain-mcp/README.md's "Tool names" section, carrying its own four-part verification record: claim, basis link, as-of date, recheck trigger).
  • Instruction-surface check. setup/SKILL.md broadens "Do not read, echo, log, copy, or persist the API key" to "any API key" (strictly broader, not weaker) and drops the Keychain/.credentials.json "never reveal" line. Not a gap: dometrain no longer holds any credential (its userConfig was removed entirely) and dometrain-mcp ships no skill at all, so neither plugin has a model-facing surface capable of reading keychain/credentials-file contents in the first place — the boundary text was removed alongside the capability it guarded, not left dangling.
  • Broadened tool-matching heuristic considered, not a new gap. Matching by server segment instead of an exact prefix is technically broader (any server exposing a dometrain segment now resolves), but the pre-existing "Namespace-collision note" already documents that a same-named colliding plugin can't be distinguished this way, and grounding/SKILL.md's standing untrusted-data instruction (treat all search_dometrain/search_code/get_lesson results as data, never instructions) already covers a spoofed/malicious server returning adversarial content — this defense is unchanged by the diff.
  • No injection or trust-boundary changes. No new command execution, no changes to the headersHelper/vault-exec opt-in mechanics (only relocated between READMEs), no permission or Actions workflow changes.

Not re-raised here: Codex's P2 comment about docs/migration-playbook.md's plugin-acceptance review record still describing dometrain as the sole remote-server plugin. That file is untouched by this PR's diff, and this lane's scope is limited to files changed in the PR — it's a legitimate documentation-currency point but outside this review's remit; the doc-drift/code-review lanes are better suited to it.

Branch: feat/dometrain-split-mcp

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 30s —— View job


Claude finished the code review

  • Gather context (PR body, diff against merge-base d15eaae, changed files)
  • Invoke /review:code-review and follow it exactly
  • Post inline findings
  • Post summary comment

Summary

Reviewed the full diff for this PR (d15eaae...428bb9a, 15 files, +480/-367) against the /review:code-review lane's criteria: correctness, CLAUDE.md/AGENTS.md alignment, maintainability, scoped to changed files.

This is the mechanical split of dometrain (skills-only, 0.5.0) into a new dometrain-mcp (bundled HTTP server + dometrain_api_key, 0.1.0). Commit 428bb9a already fixes the defect Codex flagged on grounding/SKILL.md ("Add a verification record for the MCP namespace") — the hard-coded mcp__plugin_dometrain-mcp_dometrain__* prefix in grounding/SKILL.md and setup/SKILL.md — by matching on the dometrain server segment and tool name instead, and it records the one remaining literal prefix (in the dometrain-mcp README) with a basis, as-of date, and recheck trigger per .claude/rules/skill-bodies-state-current-rules.md.

1 inline finding posted, on a changed line:

  • plugins/dometrain/skills/setup/evals/evals.json#L8 — eval 1's expected_output field still says "Checks Dometrain tool availability under both prefixes," while expectations[1] two lines below (added by the very same fix commit) says the opposite: matches by server segment/tool name, "not a fixed full prefix." Confirmed via grep -rn "prefix" plugins/dometrain/ plugins/dometrain-mcp/ that this is the only spot in the whole diff still using the old "prefixes" framing — the fix commit updated SKILL.md, the CHANGELOG.md, both READMEs, and the sibling expectations bullet, but missed this one field in the same JSON object. Posted with a suggested fix.

Everything else traced cleanly: cross-file anchor links (../../README.md#getting-the-mcp-server, #adding-your-own-server, ../dometrain-mcp/README.md#using-your-own-server-instead) all resolve to real headings; marketplace.json/settings.json/docs/catalog.md/docs/cloud-sessions.md are consistent with each other and with the fleet-enablement convention (defaultEnabled: false + explicit enabledPlugins key, matching miro/animation); version bumps and both CHANGELOG.mds line up with the described breaking change.

Not re-verified independently (author-claimed in the PR body): the shell script suite (validate-plugins.sh, check-plugin-catalog-enablement.sh, check-changelog-parity.sh, etc.), generate-catalog.mjs --check, and the skill-quality/evals checks — none of these were run in this review.

Out of this lane's scope, not re-litigated here: Codex's other finding (docs/migration-playbook.md — the trust-review record still names dometrain as the sole remote-server plugin and doesn't cover dometrain-mcp) touches a file this PR doesn't change, so it's outside this scoped-to-changed-files pass; it stands on its own merits from the Codex review.

Branch: feat/dometrain-split-mcp

Comment thread plugins/dometrain/skills/setup/evals/evals.json Outdated
…ching

Refs #5209

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H4pw3AKPP9sNePN5qwf9jL
@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 19s —— View job


Claude finished the code review

  • Gather context (PR body, diff, changed files, prior review rounds)
  • Invoke /review:code-review and follow it exactly
  • Post inline findings (none — see below)
  • Post summary comment

Summary

This is a re-review triggered by the latest push (428bb9a → f95b76a). Diffed d15eaae...f95b76a (the PR's actual merge-base, 15 files, +480/-367, matching the PR metadata) and separately diffed 428bb9a...f95b76a to isolate what changed since the last full review round.

The only change in this push is exactly the fix for the one finding raised in the prior round. Commit f95b76a is a one-line edit to plugins/dometrain/skills/setup/evals/evals.json: eval 1's expected_output no longer says "under both prefixes" (which contradicted the sibling expectations entry and the fix commit 428bb9a's own "match by server segment, not a fixed prefix" change); it now reads "by server segment and tool name, not a fixed prefix," consistent with expectations[1] on the next line and with SKILL.md/CHANGELOG.md/both READMEs.

I re-verified this is the only remaining "prefix" reference that needed to change — a repo-wide grep -rn "prefix" plugins/dometrain/ plugins/dometrain-mcp/ shows every other hit is legitimate: the CHANGELOG.md entries (historical record of the breaking change), the dometrain-mcp README's "Tool names" section (carries its own four-part verification record per .claude/rules/skill-bodies-state-current-rules.md), and setup/SKILL.md's own prose explaining why it deliberately avoids matching on a fixed prefix.

I also re-scanned the rest of the diff (marketplace.json, .claude/settings.json, docs/catalog.md, docs/cloud-sessions.md, grounding/SKILL.md, sync/context/update.md, both plugin.json manifests) for anything new since the last round — nothing has changed there, and nothing new stood out. No findings clear this lane's high-signal bar; no inline comments posted.

Not re-run here (author-claimed in the PR body, unverified by this review): the shell script suite (validate-plugins.sh, check-plugin-catalog-enablement.sh, check-changelog-parity.sh, etc.), generate-catalog.mjs --check, and the skill-quality/evals checks.

Branch: feat/dometrain-split-mcp

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 0s —— View job


Security review (/review:security-review)

  • Gather context (PR body, diff, changed files; confirmed HEAD SHA f95b76a)
  • Invoke /review:security-review and follow it exactly
  • Post findings

Skip gate: this head's only new content is a non-security wording fix.

Two full security reviews already ran on this PR and found no issues:

  • Commit 4f87520 (initial split): no issues.
  • Commit 428bb9a (server-segment matching fix): no issues, including explicit confirmation that the earlier hardcoded-prefix concern (Codex P1) was resolved without introducing a new gap.

The only change since the last review — the commit that triggered this run, f95b76a — is a single-line wording fix in plugins/dometrain/skills/setup/evals/evals.json:

- "expected_output": "Checks Dometrain tool availability under both prefixes without inspecting the token, settings, or environment variables. ..."
+ "expected_output": "Checks Dometrain tool availability by server segment and tool name, not a fixed prefix, without inspecting the token, settings, or environment variables. ..."

This brings the eval's expected_output description in line with the sibling expectations entry and the actual skill behavior (the exact gap flagged in my own code-review comment on the prior commit). It is test/eval fixture prose describing expected grading behavior, not a runtime instruction, guardrail, or credential/trust-boundary surface — no executable logic, no MCP config, no permission or secret handling touched.

No security issues found. The prior review's findings still apply in full to the current head (f95b76a): sensitive dometrain_api_key handling is unchanged (masked userConfig, sensitive: true, defaultEnabled: false on dometrain-mcp), tool-matching uses the server-segment/tool-name approach (no hardcoded namespace prefix), and no instruction-surface deletions weaken a standing guardrail without a remaining enforcement mechanism.
· Branch: feat/dometrain-split-mcp

@kyle-sexton
kyle-sexton merged commit 1438451 into main Sep 29, 2026
19 checks passed
@kyle-sexton
kyle-sexton deleted the feat/dometrain-split-mcp branch September 29, 2026 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant