Skip to content

fix(generate): preserve explicit definitions when config is supplied - #350

Merged
steipete merged 1 commit into
mainfrom
triage/20260905-generator-definition
Sep 5, 2026
Merged

fix(generate): preserve explicit definitions when config is supplied#350
steipete merged 1 commit into
mainfrom
triage/20260905-generator-definition

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

mcporter --config config.json generate-cli --command <url> discarded the explicit server definition during tool discovery. An empty config failed with “Unknown MCP server”; a conflicting same-name entry could connect to a different server from the one embedded in the generated CLI.

Always construct discovery from the already resolved definition, preserving its tool filters and metadata. The regression cases cover empty and conflicting configs using a real MCP fixture.

Restacked onto main after #348, #344, and #347 landed. This PR now contains only the code fix and regression tests. The complete 0.13.10 Unreleased section is carried by #349.

@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 4:18 PM ET / 20:18 UTC.

ClawSweeper review

What this changes

The PR makes CLI generation discover tools from the resolved server definition when a config is also supplied, with regression cases for empty and conflicting configs.

Merge readiness

Ready for maintainer review

This remains a useful, focused fix: both current main and v0.13.9 retain the faulty discovery behavior. No introduced correctness defect was found, and the supplied maintainer validation supports the intended result.

Priority: P2
Reviewed head: 38af8a205b284f74c69bbbed9878cc3c0aacefbc

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair using an existing runtime contract, with meaningful regression coverage and reported end-to-end validation.
Proof confidence 🌊 off-meta tidepool Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its validation comment additionally reports real loopback-MCP generation and generated identity calls for both config scenarios on the earlier head; exact restack equivalence was not locally verifiable.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its validation comment additionally reports real loopback-MCP generation and generated identity calls for both config scenarios on the earlier head; exact restack equivalence was not locally verifiable.
Evidence reviewed 7 items Policy and scope: Read the complete root AGENTS.md and VISION.md. No nested AGENTS.md files were found under the changed paths, and .agents/maintainer-notes is absent. The patch follows the preference for focused, verifiable fixes; no project commands or tests were executed during this read-only review.
Introduced change: The verified base-to-head delta changes one production line and adds 20 test lines. Discovery now supplies the resolved definition directly; the two added cases exercise real stdio MCP discovery, tool filtering, and derived metadata.
Existing runtime contract: createRuntime already gives explicit server definitions precedence over config loading. generateCli resolves the definition before discovery and embeds that same definition; the generated template also constructs its runtime from the embedded server. The keep-alive wrapper obtains definitions from the base runtime, so this correction preserves that established path.
Findings None None.
Security None None.

How this fits together

MCPorter generates standalone CLIs from MCP server definitions and discovered tool schemas. Discovery must contact the same server whose definition is embedded in the generated artifact.

flowchart LR
  A[CLI target and config] --> B[Resolve server definition]
  B --> C[Discovery runtime]
  C --> D[MCP server]
  D --> E[Tool schemas and metadata]
  B --> F[Generated CLI]
  E --> F
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

None.

Technical review

Best possible solution:

Keep discovery and generated execution tied to one resolved definition while preserving configured-name resolution and existing lifecycle handling.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: generate-cli with an explicit command and an empty or same-name conflicting config reaches discovery that replaces the resolved definition on main. Maintainer-reported before/after failures corroborate this; this review did not execute tests.

Is this the best way to solve the issue?

Yes. Using the existing explicit-server runtime option is the narrowest repair and aligns discovery with the definition already embedded in generated CLIs.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 7a2d4dc563c9.

Labels

Label justifications:

  • P2: This fixes a bounded CLI-generation failure when an explicit server is combined with an empty or conflicting config.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its validation comment additionally reports real loopback-MCP generation and generated identity calls for both config scenarios on the earlier head; exact restack equivalence was not locally verifiable.

Evidence

What I checked:

  • Policy and scope: Read the complete root AGENTS.md and VISION.md. No nested AGENTS.md files were found under the changed paths, and .agents/maintainer-notes is absent. The patch follows the preference for focused, verifiable fixes; no project commands or tests were executed during this read-only review. (AGENTS.md:1, 38af8a205b28)
  • Introduced change: The verified base-to-head delta changes one production line and adds 20 test lines. Discovery now supplies the resolved definition directly; the two added cases exercise real stdio MCP discovery, tool filtering, and derived metadata. (src/cli/generate/definition.ts:134, 38af8a205b28)
  • Existing runtime contract: createRuntime already gives explicit server definitions precedence over config loading. generateCli resolves the definition before discovery and embeds that same definition; the generated template also constructs its runtime from the embedded server. The keep-alive wrapper obtains definitions from the base runtime, so this correction preserves that established path. (src/runtime.ts:140, 38af8a205b28)
  • Still necessary on main: The fetched main revision still passes undefined servers whenever configPath is present, allowing discovery to reload an empty or conflicting config instead of using the resolved definition. (src/cli/generate/definition.ts:134, 7a2d4dc563c9)
  • Latest release check: The v0.13.9 version of fetchTools contains the same config-dependent definition replacement. The supplied related merged PRs concern fixture timing, URL tests, and daemon spawn errors; none implements this correction. (src/cli/generate/definition.ts, 64d613c1e96f)
  • Maintainer validation and review continuity: The captured comment at fix(generate): preserve explicit definitions when config is supplied #350 (comment) reports both regressions failing before the fix, passing check/full tests afterward, and successful built-CLI generation plus identity calls against a loopback MCP server for empty and conflicting configs. This validation names the earlier head 60eb49d. That object was unavailable locally and GitHub DNS resolution failed, so exact earlier-head equivalence could not be independently checked. The prior review projection contains no findings or published rank-up moves. (60eb49dd9e9a)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-05T19:58:22.746Z sha 60eb49d :: needs maintainer review before merge. :: none

@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Maintainer validation on 60eb49dd9e9a56ee945db073a5a5e4f401cddea7 is complete.

Both new regression cases failed before the fix: an empty config produced “Unknown MCP server 'fixture'”, and a conflicting same-name config launched the wrong process and closed the connection. After the fix, pnpm check and full pnpm test passed (217 files, 1,863 tests; 26 skipped). Existing keep-alive discovery coverage remains passing. Independent local and committed-branch autoreview through P2 are clean.

Real CLI proof: node dist/cli.js --config <synthetic-config> generate-cli --command <loopback-MCP-URL> --name synthetic-proof --runtime node --bundle <artifact> --output <template> successfully produced a Node bundle for both an empty config and a same-name config pointing at a different endpoint. Running each generated artifact's identity --output json returned the intended synthetic server result. The configured entry never replaced the explicit discovery target.

Exact-head Linux, macOS, and Windows CI passed: https://github.com/openclaw/mcporter/actions/runs/33988606910

The CLI generator documentation and 0.13.10 Unreleased entry now describe the preserved definition precedence. Ready for squash landing.

@steipete
steipete force-pushed the triage/20260905-generator-definition branch from 60eb49d to 38af8a2 Compare September 5, 2026 20:14
@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Restacked onto 7a2d4dc563c944b7d8725fd070b727dea30600b2 at head 38af8a205b284f74c69bbbed9878cc3c0aacefbc. The diff now contains only the discovery fix and regression tests; CHANGELOG and documentation are unchanged from main. The complete release notes are in #349.

pnpm check passed; the full suite passed all 1,871 tests (26 existing skips), using two workers on the shared host. The built CLI again generated and executed the intended synthetic HTTP tool with both an empty config and a conflicting same-name config. Committed-branch autoreview through P2 is clean.

Exact-head Linux, macOS, and Windows CI passed: https://github.com/openclaw/mcporter/actions/runs/33989501417

@steipete
steipete merged commit 7ac6cd1 into main Sep 5, 2026
8 of 9 checks passed
@steipete
steipete deleted the triage/20260905-generator-definition branch September 5, 2026 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant