Refine MCP import reconciliation and validation

[idiotsj]

Summary

• add a shared MCP import reconciliation helper so Claude/Codex/Gemini/OpenCode follow the same merge policy
• canonicalize MCP specs at import and persistence boundaries, including http_headers -> headers normalization and strict remote type handling
• return structured import results with conflict/invalid reporting and surface them in the unified MCP import UI
• tighten Codex TOML import/export behavior and add coverage for reconciliation, validation, and frontend toast/parse flows
• fix the existing UnifiedSkillsPanel test mock drift and clean up unrelated Prettier formatting so the documented frontend checks pass cleanly

Validation

• pnpm typecheck ✅
• pnpm format:check ✅
• pnpm test:unit ✅
• pnpm exec vitest run tests/hooks/useMcpValidation.test.tsx tests/utils/tomlUtils.test.ts tests/components/UnifiedMcpPanel.test.tsx ✅
• Rust tests not run in this environment because cargo/rustfmt are unavailable

Notes

• README contribution guidance asks contributors to run pnpm typecheck, pnpm format:check, and pnpm test:unit
• This PR is a bugfix/reconciliation change rather than a net-new feature

[farion1231]

Thanks a lot for your contribution — this is a solid improvement overall 👍
I’ve gone through the changes and would summarize my feedback as 2 blocking issues, 1 suggested fix, and 2 notes:

Blocking issues

1. Fail-fast aggregation without transaction rollback

   The aggregated import is now fail-fast, but earlier sources may have already been partially persisted. This can lead to a mismatch between the reported failure and the actual state.

   Relevant code:

   • services/mcp.rs#L246
   • commands/mcp.rs#L199

   Previously, the logic tolerated errors per source and continued importing. Now, a single source failure causes the whole process to return an error, but without any rollback. This should either be wrapped in a transaction or reverted to a tolerant strategy.

2. “All apps disabled” treated as unowned entry

   Treating entries with all apps disabled as “unowned” can result in silently overwriting local specs when importing the same ID from external sources.

   Relevant code:

   • importing.rs#L135

   From a user perspective, disabling all apps does not mean giving up ownership of the configuration. This behavior may lead to unexpected data loss.

---

Suggested improvement

3. Swallowing normalization errors with .ok()

   When normalizing existing servers, using .ok() may hide invalid legacy data and treat it as a normal spec difference.

   Relevant code:

   • importing.rs#L153

   This could lead to incorrect overwrite/conflict decisions. I’d consider handling this explicitly instead of silently ignoring errors.

---

Notes

4. Behavior change: missing type now errors

   Previously, missing type (e.g. for url) defaulted to stdio. Now it throws an error. This is a clear behavior change — it would be good to document it in the changelog or release notes.

5. Import order affects conflict resolution

   The current implementation makes conflict resolution dependent on import order across apps. It might be worth documenting the priority or making it explicit in tests.

---

One more note: the new tests look great and are all passing, but they don’t currently cover the two blocking regression scenarios above.

Let me know what you think — happy to discuss possible approaches 🙂