Repository navigation
fix(mcp): keep Muse ownership across JSON key reordering - #1346
Conversation
|
No findings in the specified PR diff. The PR description documents sufficient testing, including a build followed by 7 real-CLI E2E tests covering immediate uninstall after a settings rewrite. A live Muse save and full provider × agent coverage are not required. Review was read-only; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks, approving. This closes the key-order half of the #1221 Muse item.
We built this head, its merge-base 83f6d1db and main 54622c3a. We installed a stdio server with nested env and an HTTP server with nested headers, then simulated a Muse save that reverses the keys at every level and drops stdio's inferred type.
- base and main:
uninstallkeeps both servers as "you changed it". - this head:
uninstallremoves both, and a server whose command was really edited is still kept.
Upgrade and downgrade both hold:
- records written by base are recognized by this build, both before and after a pull adds
valueHash; - base still reads records this build wrote, and only falls back to its order-sensitive judgment.
Non-Muse formats keep their ownership rules, since the new check only runs for format === 'muse' and entryHash is unchanged. The MCP unit files (397 tests) and muse-support pass, and the new tests fail on base. tsc, lint and CI pass.
Suggestions:
[P2 non-blocking]Merging with #1345 needs one line. We built this head merged with #1345 (c15f4f21). An unchanged legacy stdio server is then kept on uninstall, andmuse.test.tsfails its legacy case, although all 8muse-supporte2e tests pass. #1345 judges a normalized copy, so the legacy fallback here has to judge that copy too:With that change, all 41 Muse unit tests pass and the upgrade run removes both servers. Whichever of the two lands second needs it.- : await judge(name, doc.servers[name], previous.hash) === 'teamai' + : await judge(name, comparable.servers[name], previous.hash) === 'teamai'
[P3 nit]moveToLocalScoperebuilds records withoutvalueHash. That only reaches Claude and CodeBuddy targets, which never read it, so no decision changes today. The Muse stdio inference also lives in two modules.
CI review: it reported no findings and accepted the testing record. We agree for this PR on its own. The interaction with #1345 above is outside what it could see.
#1345 merged first, so this conflicts with - : await judge(name, doc.servers[name], previous.hash) === 'teamai'
+ : await judge(name, comparable.servers[name], previous.hash) === 'teamai'Please rerun |
fc0bc34 to
fddda28
Compare
|
Addressed the review follow-up at
The PR description contains the updated verification record. Ready for another review. |
|
No findings in the specified PR diff. The PR description documents sufficient testing: a build followed by 8 built-CLI E2E tests, including immediate uninstall after a simulated Muse settings rewrite and legacy upgrade. A live Muse save and full provider × agent coverage are not required. The earlier review reported no findings, so there are no prior findings to mark resolved. Review was read-only; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks, approving again at fddda28. The legacy fallback now judges with comparable.servers[name], as #1345 needed. On the rest of the change, the earlier approval holds. With the built CLI, a server whose keys Muse reordered (and whose inferred type it dropped) is removed by uninstall --agent muse on this head and kept on the base. A real command edit is still kept with a warning. Upgrade from a base install and downgrade to the base both behave. The new unit tests and the new e2e case fail on the base. Build, tsc, lint, the PR's unit files (139), muse-support (8/8) and the 16 e2e files that touch MCP removal pass.
It now conflicts with main in src/__tests__/e2e/muse-support.test.ts, after #1344 added its cases to the same file. Keeping both sets of cases resolves it, and I'll merge it once it's rebased and green.
- [P3 nit]
moveToLocalScopestill rebuilds records withoutvalueHash(src/local-agent.ts). Muse never reaches that path today. The Muse stdio inference lives in bothmcp-format.tsandmcp-reconcile.ts. The newsharing.mdparagraph repeats the troubleshooting rule, and the body dates its follow-up 2026-10-25.
The CI review reports no findings. We agree. All CI jobs pass.
- Keep legacy rendered hashes intact and add a Muse-only content fingerprint that sorts nested object keys, infers omitted stdio types and preserves meaningful array order. - Record the same fingerprint from Git reconciliation and HTTP MCP installs. During CLI removal, recognize equivalent saves while keeping real command, argument, environment and header edits. - For legacy records, require a team-history render matching both the original hash and installed value; keep unverifiable entries rather than treating any historical value as ownership proof. - Verify 128 Muse/HTTP MCP unit tests and 7 built-CLI Muse E2E cases, including immediate uninstall after key reordering, legacy-history boundaries and true edits; run TypeScript/lint and update bilingual guidance. Refs: Tencent#1221
fddda28 to
4218e88
Compare
|
No findings in the specified PR diff. The PR description documents sufficient testing: build plus representative built-CLI E2E verification, including immediate uninstall after a simulated Muse settings rewrite. A live Muse save and full provider × agent coverage are not required. Earlier reviews reported no findings, so none need marking resolved. Review was read-only; I did not run, build, or install anything from the PR. I reviewed the requested diff ending at |
|
Updated and rebased onto main Verification: 396 focused unit tests passed; types, lint and build passed. Post-build real-CLI Updated head: |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Re-approving at 4218e88. The PR's own patch is unchanged from fddda28. It rebases cleanly on current main, and tsc plus the PR's unit files pass on the merged tree. Rechecked on this head:
- The three unit files pass (396), and the
muse-supporte2e passes (18). - Real CLI: with a reordered Muse save,
uninstallremoves the server. A real edit to the entry keeps it and prints the warning. - All CI jobs pass at this head.
The legacy-fallback P2 from the first round is fixed (comparable.servers[name] in src/mcp-reconcile.ts). The CI review reports no findings. We agree.
[P3 nit] The commit message says 128 unit and 7 e2e tests. The body and our runs say 396 and 18. That doesn't matter for a squash merge.
Rebased onto upstream main (6 commits landed since this branch was cut, including Tencent#1346 which added these .reverse() calls). Same fix as the rest of this PR: switched to Array#toReversed(), since none of these sites reuse the original array reference after reversing it.
Rebased onto upstream main (6 commits landed since this branch was cut, including Tencent#1346 which added these .reverse() calls). Same fix as the rest of this PR: switched to Array#toReversed(), since none of these sites reuse the original array reference after reversing it.
A Muse save that merely reorders JSON object keys makes the order-sensitive legacy
entryHashlook like a member edit.teamai uninstallthen retains the server even though its content is unchanged.Keep the existing rendered hash for compatibility, and record a Muse-only content fingerprint from both Git reconciliation and HTTP MCP installation. It ignores object key order and an inferred stdio type, including nested object order, while keeping arrays and actual content changes meaningful. CLI removal uses that fingerprint; older records require a historical render matching both their original hash and installed value. Unverifiable legacy records remain in place with the existing warning.
Review follow-up:
Rebase only: keep both the newer Muse anchoring cases and this PR's reordered-server uninstall case. Retain the comparison-only legacy ownership view required by #1345. No additional runtime change was needed.
Validation on this branch, rebased onto
fc8a229e:npx tsc --noEmit,npm run lint,npm run build,git diff --check: passed.npx vitest run src/__tests__/muse.test.ts src/__tests__/mcp-reconcile.test.ts src/__tests__/local-agent-mcp.test.ts: 396 passed.npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/muse-support.test.ts: 18 passed, using the real CLI and isolated local Git/HOME fixtures.Local host: macOS, Node 22.22.1, Apple Git 2.50.1. Native Windows, live vendor applications and additional provider combinations were not run locally and are left to CI. This record replaces the earlier combined-snapshot validation paragraphs.
Refs #1221; this PR addresses only its stated independent defect.