Repository navigation
feat(local-agent): serve MCP and models to OpenClaw, Hermes and DSH - #1809
Conversation
Testing: The description documents a representative manual end-to-end record, so no missing-E2E finding. I only read the diff; I did not run, build, or install PR code. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Reviewed at a7e7e2cf. A single pull delivers well: the servers land in openclaw.json, config.yaml and cordis.patch.yml, and the member's own entries and comments survive. A second delivery of the same server adds no duplicate. These three files are now shared by every workspace on the machine and by user scope, though, and two paths still treat them as one workspace's own. Both reproduce with the built CLI.
What I ran (built a7e7e2cf and the merge-base d54415d6, isolated HOME, empty GIT_CONFIG_GLOBAL, GIT_CONFIG_NOSYSTEM=1; team with root mcp/mcp.yaml (docs-mcp, local-tools) and mcp/be/mcp.yaml (be-only), roles fe and be; each tool's home seeded with a member entry mine and comments):
1. workspace app1, role fe: teamai init <url> --agent openclaw --agent hermes --agent dsh --role fe; teamai pull exit 0
-> docs-mcp and local-tools in all three files; mine and the comments kept. Merge-base build: nothing written.
2. workspace app2, role be, same steps exit 0
-> be-only added to all three files.
3. team drops docs-mcp; teamai pull in app1 exit 0
-> "MCP: 6 change(s) across 2 server(s)": docs-mcp and be-only removed from all three files;
app2's managed-mcp.json still names be-only.
4. user-scope install and pull, then workspace app1 with the same servers; teamai uninstall --force outside it
-> "Removed 6 teamai-managed MCP server(s)": docs-mcp and local-tools gone from all three files, while
app1's record still names them; the "Kept for the projects still set up" list does not mention them.
Blocking
- [P1 blocking] A pull in one workspace deletes another workspace's server (step 3). The removal loop in
src/mcp-reconcile.tstreats an unrecorded entry that matches a render from the team history as teamai's, and deletes it unless this workspace's own record (ownedNames) names it. Now that a project's servers go to a user file (mcpScopeFor,src/tools.ts), that file has other owners, and the loop never reads their records. Until app2 pulls again, an agent started there runs withoutbe-only. Counting a server as owned when any record of the file names it, whatever scope or workspace wrote that record, would fix this.shadowedMcpRecordsalready reads the other records. - [P1 blocking] A user-scope uninstall removes servers a set-up workspace still delivers (step 4).
src/uninstall.tscallsreconcileMcpForConfig(..., { removeAll: true }), which reaches the same loop for entries app1 also records. The same rule (keep what another live record names) would cover it. The message would then be true when it says the project is kept.
Non-blocking
- [P2 non-blocking] The OpenClaw warning says teamai left
openclaw.jsonas it is, in the same pull that writes MCP servers into it (src/openclaw-hooks.ts:458). On main the sentence was true, but with this PR it only holds for the hook entry. - [P3 nit] Changes outside the three tools that the body does not mention.
restoreMcpDocnow restores the exact bytes on a failed write for every JSON MCP tool. This is an improvement, but the body says no other tool's behaviour changes.parseYamlno longer warns about!!jsin any YAML file it reads. The value still stays a string.
CI review
The CI review at this head reports three [P1 blocking] items. I read each one in the code and agree with all three:
- A changed API key or base URL is refused for teamai's own provider (
src/local-agent.ts:6971).planOpenclawcompares the provider'sbaseUrlandapiKeywith the new delivery and throws "a different API key" or "a different base URL" before it looks at the record. A team that changes its key or endpoint therefore leaves every member on the old one. The Hermes path does the same. When the record shows teamai wrote those fields, replacing them should be allowed. - Re-delivery from workspace to user scope is refused (
src/local-agent.ts:5124). The lookup of the other scope's records (cross) runs only whenprojectScopeis set. A server installed at workspace scope and then delivered at user scope is still recorded only under the workspace key, so the user install treats teamai's own entry as the member's and refuses it. The body's idempotence check covers the other order only. The lookup needs to run in both directions. - Model files are written before their record is saved (
src/local-agent.ts:4591). This matches what I found.applyModelConfigcalls the threereconcile*Modelsfunctions without thecommitargument they accept, then writes the manifest. If that write fails, the member's file keeps the new models while the record keeps the old hashes, and a later delivery leaves them behind as the member's own. Saving the record throughcommitwould roll the file back on failure.
Before CI can run
The branch conflicts with main in CHANGELOG.md, both tool-support.md pages and src/tools.ts, so no CI run exists for this head. A rebase is needed before lint, the unit suite and the E2E shards can say anything. The body's real-client run in Docker is useful, but it has no commands, exit codes or commit. The record above can go in the body, with the real-client lines added to it.
Merge risk
Merge risk: high.
- Alignment: decent. The pull path matches the body. The two scope paths above do not, and the body does not mention the changes outside the three tools.
- Impact: large. The PR rewrites three tools' user configs, which every workspace on the machine shares, in a 3.6k-line diff.
- Verification: poor. The record has no commands, and neither it nor the tests cover two workspaces or uninstall. CI cannot run until the branch is rebased.
install_mcpandapply_model_configthrough the local-agent backend were not run here. - Rollback cost: medium. The configs are restored by the next pull or uninstall, but those are the two paths that delete live entries.
- Error detection: decent. A file that cannot be parsed is left unchanged with a warning. Gaps are the OpenClaw warning above and the model record.
@jeff-r2026 your call on this one. It is a new capability that writes into files every workspace on a machine shares, so the ownership rule for those files is a design decision as much as a fix.
a7e7e2c to
1797833
Compare
|
All five blocking items are fixed on
The branch is rebased on the current main, so CI can run. |
Earlier findings: Workspace-to-user MCP re-delivery and transactional model-manifest persistence are resolved. Credential rotation is partially resolved, with the DSH endpoint issue remaining above. Testing: The description records representative real-CLI end-to-end verification, so no missing-E2E finding. I only read the diff; I did not run, build, or install PR code. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Re-reviewed at 17978333. Thanks for the quick round. These are now resolved:
- the OpenClaw warning;
- the body's list of changes outside the three tools;
- re-delivery from workspace to user scope through
install_mcp; - the model record saved through
commit; - the key and URL change for OpenClaw and Hermes.
I checked each one in the code. The two shared-file P1s still reproduce with the built CLI, though, and the provider change has two gaps.
Still blocking
-
[P1 blocking] A pull in one workspace still deletes another workspace's server, and a user-scope uninstall still removes servers a workspace records. I reran both setups from the first round on this head: a role-
fepull removesbe-onlyfrom all three files while thebeworkspace still records it, anduninstall --forceoutside the workspace prints "Removed 6 teamai-managed MCP server(s)" for servers the workspace still records. The cause is the record key:- These three tools have no project file, so a workspace's pull records its servers under the plain tool key,
{"openclaw": [...]}, in that workspace'smanaged-mcp.json. namesRecordedElsewhere(src/mcp-reconcile.ts) reads${tool}:projectfrom each workspace shard, which is the keyinstall_mcpwrites, and so it never sees a record a pull wrote.
A reader that takes both keys would close the unrecorded sweep. The CI review also points out that the loop over this run's own recorded entries removes them whatever other records say. So a server that user scope adopted, and that a workspace also records, is still deleted on user uninstall. That loop needs the same check.
- These three tools have no project file, so a workspace's pull records its servers under the plain tool key,
New blocking
- [P1 blocking] A member's own URL or key on the teamai provider is now overwritten without a word.
teamaiProvider(src/local-agent.ts:7009, and the Hermes counterpart at:7213) counts the provider as teamai's when every model matches its record. It never looks at the provider's ownbaseUrlorapiKey.- The write then sets both from the delivery.
- So a member who replaced the key in
~/.openclaw/openclaw.jsonand left the models alone gets the team's key back on the next delivery, with no message. Main refused this case with "a different API key". - A
containerHashis already recorded for the provider, but only the removal path reads it. Checking it here too would let a team's change through and still stop at a member's.
- [P1 blocking] DSH still refuses a changed base URL (
src/local-agent.ts:7714).planDshthrows "a different baseURL" with no ownership check, so a team that moves its endpoint leaves DSH members on the old one. It needs the same rule as the other two tools, including the check above. - [P1 blocking] An empty delivery drops the API-key reference of a model the member edited (CI review,
src/local-agent.ts:7774). I confirmed this in the code. When nothing is delivered and no managed model was removed,planDshreturns{ write: null }before it setsretainsProvider. The cleanup at:8206keeps the key store only for a plan that retains its provider, so the kept model loses the reference it uses. SettingretainsProviderwhenever the provider and a kept model remain would fix it. I did not run this one.
CI review at 17978333
It reports four P1s, and I agree with all four. The joint-ownership one reproduces in the order I ran: user install, workspace pull, then user uninstall. The DSH URL, provider-edit and empty-delivery items are from reading the code.
Not run
install_mcpandapply_model_configthrough the local-agent backend, so every model-path item above comes from the code.- CI has not run on this head yet: the fork's workflow runs are waiting for approval.
Merge risk
Merge risk: high. The PR still deletes live entries in a file every workspace shares. The new provider rule gives up a refusal that main had.
@jeff-r2026 your call on this one. The shared-file ownership rule is the design question, and this round shows it needs both record keys and both removal loops to agree.
1797833 to
081348d
Compare
|
Second round, on
|
Earlier findings: Model credential/endpoint rotation, provider-field ownership checks, model-manifest transaction persistence, and member-edited DSH model credential retention are resolved. Command-to-command workspace/user re-delivery is also resolved. Shared-scope MCP cleanup remains incomplete as described above. Testing: The description documents representative real-CLI end-to-end verification, so no missing-E2E finding. I only read the diff; I did not run, build, or install PR code. |
081348d to
16154a4
Compare
|
Both remaining items are fixed on
|
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Re-reviewed at 16154a4a. The provider fixes hold:
- a member's own key on teamai's provider is now refused ("already has a different API key; teamai does not overwrite it");
- a team key or URL change still goes through;
- the DSH
baseURLmoves to the new URL.
The shared-file ownership still fails in the runs below. One root cause explains three of them, and this head adds a regression.
A correction to my last review: I wrote that main refused a member's own key with "a different API key". Main has no model delivery for these three tools, so that comparison was wrong. The finding stands on its own: the member's key must not be overwritten, and this head now refuses that case.
Blocking
-
[P1 blocking] The cross-scope readers look in a directory that does not exist.
namesRecordedElsewherereadsgetDataHome(localConfig)/workspaces(src/mcp-reconcile.ts:2741), andloadWorkspaceScopeMcpManifestsreadsresolveDataHomeForScope('user')/workspaces, which is~/.teamai/workspaces(src/local-agent.ts:5015). A workspace's record lives under its project, at~/.teamai/projects/<slug>/workspaces/<id>/managed-mcp.json. So no other project's record is read, and~/.teamai/workspacesis never there. This one cause still produces all of these:- A pull in one workspace removes another workspace's server. After the role-
fepull,be-onlyis gone fromopenclaw.jsonwhile thebeworkspace still records it. - A user-scope
uninstall --forceremoves servers a workspace records. It prints "Removed 6 teamai-managed MCP server(s)" while the workspace record still namesdocs-mcpandlocal-tools. - A user-scope
uninstall_mcpremoves an entry a workspace still records (CI review). Installdocs-mcpat user scope, then at workspace scope, then rununinstall_mcpat user scope: the entry is gone.slugOwnedInOtherScopefinds no shard, so it returns false. - A user-scope
install_mcprefuses teamai's own entry when a workspace records the server (CI review). It says "exists in openclaw config and is not managed by teamai". This happens both after a workspace pull and after a workspaceinstall_mcp.
projectsRootDir()already exists insrc/mcp-reconcile.ts. Scanningprojects/*/workspaces/*/managed-mcp.jsonfrom there, with both the plain and the:projectkey, would reach every record. - A pull in one workspace removes another workspace's server. After the role-
-
[P1 blocking] New in this head: a server the team drops stays forever once its only owner pulls. One workspace with role
fepullsdocs-mcp, the team drops it, and that workspace pulls again.openclaw.jsonstill hasdocs-mcp, and no record names it, so uninstall cannot remove it either. At081348d6the entry is removed. The plain-key read innamesRecordedElsewhereincludes the workspace's own shard, so its own record counts as another owner and leavesowned. Skipping the current workspace's shard in that read would fix it. The two-workspace drop keepsdocs-mcpwith no record for the same reason.
Non-blocking
- [P2 non-blocking] The body's real-CLI record says a second-scope install moves the record. This head keeps both. The record should be rerun once the items above are fixed.
CI review
The last CI review comment was written before this push. It reported two P1s:
- records written by a workspace pull use the plain key;
uninstall_mcpat user scope ignores the workspace's record.
This head reads the plain key, but both still reproduce here because of the directory above. I agree with both.
Not run
CI has not run on this head: the fork's workflow runs are waiting for approval.
Merge risk: high.
@jeff-r2026 your call on this one. The provider side is in good shape. The open question is still who owns an entry in a file every workspace shares, and the code needs to read records where they are written.
Earlier findings: Model rotation, provider-field ownership checks, model-manifest transactions, and member-edited DSH credential retention are resolved. Both workspace key forms are recognized by cross-scope readers, but MCP ownership remains incomplete in the cases above. Testing: The description documents representative real-CLI end-to-end verification; no missing-E2E finding. I only read the diff and its context; I did not run, build, or install PR code. |
16154a4 to
a8fcbe9
Compare
|
Fixed on
|
Resolved: Project-partition discovery and exclusion of reconciliation’s own manifest are fixed. The earlier model rotation, provider-field ownership, transaction, and credential-retention findings remain resolved. Testing: The description documents representative real-client end-to-end verification; no missing-E2E finding. I only read the diff and context, without running, building, or installing PR code. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Read a8fcbe99. loadManagedMcpShards now reads records where they are written, under both key forms, and namesRecordedElsewhere skips the run's own manifest. That covers both items from my last review. I'll rerun the two-workspace and user-uninstall setups on the head that settles the points below.
I checked the four P1s in the CI review at this head in the code, and I agree with them:
- Workspace commands read only
<tool>:projectfrom their own manifest (src/local-agent.ts:5150,records = manifest[manifestKey]). A server a workspace pull recorded under the plain key is invisible toinstall_mcpanduninstall_mcpin that same workspace: the install refuses teamai's own entry, and the uninstall leaves it. The current manifest needs both keys, as the cross-scope readers now have. - A workspace
uninstall_mcpalso removes the user owner's record. The loop overlocationsincludes the user manifest (cross). Each pass sees the other record as a surviving owner and keeps the entry, then drops that pass's record. With both records gone, the entry is no longer managed. Only the uninstalling scope's record should go. - A workspace
uninstall_mcpchecks only the user manifest for other owners (slugOwnedInOtherScope, theprojectScopebranch). Suppose workspace A installed a server by command and workspace B pulled the same one. Uninstalling it from A then deletes the entry that B still records. That branch needs the other workspaces' shards too, without A's own. - A record goes stale when another scope changes the shared entry (
src/local-agent.ts:5152). After a user-scope install rewrites the entry, the workspace record keeps the old hash. A later workspace install then treats the matching user record as replaced by its own, and refuses the entry. I followed this one in the code less fully than the other three.
These four come from reading the code; I have not run them. The first three are one rule applied in three places: every path that adds or removes a record of a shared file must read all owners' records, by both key forms, without its own.
CI has not run on this head yet: the fork's workflow runs are waiting for approval.
@jeff-r2026 your call on this one. These four points are still about who owns an entry in the shared file.
a8fcbe9 to
0834ae0
Compare
|
All four are fixed on
Checked with the built CLI in Docker,
|
Earlier findings: The previously reported model issues, workspace-key handling, and uninstall protection scenarios are resolved. The shared-update cleanup case above remains distinct from the fixed re-install scenario. Testing: The description documents representative real-client end-to-end verification; no missing-E2E finding. I only read the diff and its context—no PR code was run, built, or installed. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Re-reviewed at 0834ae07, with the built CLI. The four command-path P1s from the CI review at a8fcbe99 are fixed. So are the earlier shared-file items: a pull no longer removes another workspace's server, a server the team drops goes once its last owner pulls, and both user-scope install_mcp and uninstall_mcp respect a workspace's record. Two cases still fail in the runs below, and the CI review at this head adds two more.
Blocking
-
[P1 blocking] A deleted workspace keeps a dropped server forever. Setup:
app1andapp2pulldocs-mcpinto OpenClaw.- The team drops it.
app2is deleted withoutteamai uninstall, as a removed worktree is.app1pulls.
openclaw.jsonstill hasdocs-mcp, andteamai uninstall --forceinapp1keeps it too. The cause isloadManagedMcpShards: it lists everyprojects/<slug>/workspaces/<id>/managed-mcp.jsonwithout checking that the workspace still exists.namesRecordedElsewhere, the pull guard andslugOwnedInOtherScopeall count that record as a live owner, so the entry never loses its last owner. Counting a shard only while its workspace path exists, or pruning it when the workspace goes, would give it an end. -
[P1 blocking] A workspace
install_mcprefuses a server another workspace's pull recorded (the CI review's first P1 at this head names the same gap). Afterapp2pullsdocs-mcp,install_mcp docs-mcpforapp1fails with "server "docs-mcp" exists in openclaw config and is not managed by teamai". On the workspace path,crossreads only the user manifest, andworkspaceCrossesis built only when!projectScope(src/local-agent.ts:5146). So no other workspace's record reachesowned. The install path needs the same all-owners read that uninstall now has. -
[P1 blocking] A workspace that keeps its data home inside the workspace is found only when the local agent is enrolled in it (CI review).
loadManagedMcpShardsreaches<workspace>/.teamai/workspaces/<id>/only throughworkspaceBindingsin the local agent's config. A self-mode workspace that runsteamai pullwithout enrolling is invisible, so its servers have no owner seen from anywhere else. I confirmed this in the code; I did not run it. -
[P1 blocking] A scope's record can go stale when another scope's entry wins (CI review,
src/local-agent.ts:5459). Setup:- A server is installed at URL A at workspace scope, then at URL B at user scope.
- The user scope uninstalls it.
The guard keeps entry B but drops the user record. Only the workspace's hash of A is left, so the workspace's own uninstall then treats B as a member edit and leaves it. This follows from the code as the CI review describes; I did not trace every branch.
Non-blocking
- [P2 non-blocking] "Removed N teamai-managed MCP server(s)" counts records, not entries that left the config: it says "Removed 2" while
docs-mcpstays. The line predates this PR, but this PR makes the two numbers differ. - [P3 nit] A shard that cannot be read counts as "no owner" (
readJson(...).catch(() => null)). That error leads to a delete, not a keep. I found this by reading the code.
Before CI can run
CI has not run on this head, because the fork's workflow runs are waiting for approval. The branch is on 53ad0e66, and main has moved since. It merges cleanly, but a rebase would let CI run on what will be merged.
Merge risk: high. Each round closes the cases I listed and finds the next one: an entry in the shared file still has no single place that decides who owns it.
@jeff-r2026 your call on this one. After five rounds, the open items are all about one invariant: one owner set per shared entry, read the same way by every path that adds or removes a record. Whether that shape fits the local-agent design is the maintainer's decision.
OpenClaw, Hermes and DSH have no project MCP file and no project model store. Deliver a workspace's MCP server to the config each tool reads (its user config) while keeping the workspace's ownership record, and apply delivered models to the provider store each tool boots from. - tools: add `mcpScopeFor`, so a tool with no project MCP channel takes a workspace's server in its user config instead of skipping it - locate/mcp-reconcile: resolve those servers against the user channel, with the user channel's syntax (OpenClaw's JSON5 config) - local-agent: install/uninstall read the other scope's records for the same file and adopt them, so a re-delivery at another scope no longer fails with "exists ... and is not managed by teamai" - local-agent: apply_model_config writes the OpenClaw, Hermes and DSH model stores; workspace scope is ignored for them, as none has one - hooks list: report dsh from its adapter files instead of "not configured" - yaml: read DSH's `!!js` tag as its source text, without the warning
0834ae0 to
7743ace
Compare
Earlier findings: Cross-workspace installation and non-enrolled self-mode discovery are resolved. Command-driven shared-update cleanup is fixed, but its reconciliation counterpart remains above. The previously resolved model and scope-handling findings remain resolved. Testing: The description documents representative real-client end-to-end verification; no missing-E2E finding. I only read the specified diff and its context—no PR code was run, built, or installed. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Re-reviewed at 7743ace7 with the built CLI. The client configs were stand-ins. Two of the four blocking items from my last review are fixed:
- A deleted workspace no longer protects a dropped server. With
app2removed,app1's pull takesdocs-mcpout. - A workspace
install_mcpnow accepts a server another workspace's pull recorded.
I traced a third in the code without running it: a data home inside the workspace is now found without enrollment, through managed-mcp-homes.json. The fourth, the stale hash, is fixed on the command path but not on the reconcile path (first item below). The CI review at this head raised the three blocking items below, and each one reproduced here.
Blocking
-
[P1 blocking] A user-scope
teamai uninstall --forcedrops the user's record for a name a workspace also records, and the workspace then treats the entry as a member edit. Setup:- a workspace pull installs
docs-mcpat URL A; - a user-scope install puts it at URL B;
- user-scope
uninstall --forceruns.
B stays, which is right, but the user record goes and the workspace keeps hash A. The workspace's
uninstall --forcethen prints "Kept MCP server docs-mcp … you changed it since teamai wrote it", and B is left with no owner. InreconcileTargets, the shared name is kept out ofowned(src/mcp-reconcile.ts:3486), and no remaining owner's hash is updated. The command path handles this withadoptKeptMcpEntry; reconcile needs the same step. - a workspace pull installs
-
[P1 blocking] A server a workspace installs with
install_mcpsurvives that workspace's ownuninstall --force. Setup:- http mode,
bind-project; - workspace init;
- workspace
install_mcp docs-mcp; - workspace
teamai uninstall --force.
It prints "Removed 1 teamai-managed MCP server(s)" and removes the record, but
openclaw.jsonstill hasdocs-mcp. The record is underopenclaw:project, which matches the CI review's point that reconcile reads only<tool>. I did not trace the write. - http mode,
-
[P1 blocking] A workspace uninstall deletes a member's edit after the user scope leaves. Setup:
- an identical
docs-mcpis installed at workspace and user scope; - the member changes its URL in
openclaw.json; - user-scope
uninstall_mcpruns, then workspaceuninstall_mcp.
The first uninstall keeps the edit. The second removes it.
adoptKeptMcpEntry(src/local-agent.ts:5349) copies the current entry's hash into the other owners' records whatever it is. Copying it only when the entry matches a record teamai wrote would keep the edit. - an identical
Non-blocking
- [P2 non-blocking] "Removed N teamai-managed MCP server(s)" still counts records, not entries that left the config. In the first case above it says "Removed 2" while
docs-mcpstays. - [P3 nit]
completeis read inverted, so an unreadable record fails open. TheMcpOwners.completecomment means false is "an owner may be missing, keep everything". The install refusal fires only whencompleteis true (src/local-agent.ts:5140,:5160). Reconcile's filter puts records inownedwhen it is false (src/mcp-reconcile.ts:3486), andownedis what gets removed. With an unparseablemanaged-mcp.json, a user-scopeinstall_mcp mineoverwrote the member's ownmineentry, which main refuses. A corrupt record is unlikely, but the fix is to flip the condition at these three sites.
Before merge
CI has not run at this head, because the fork's workflow runs are waiting for approval. The branch is on 53ad0e66 and merges cleanly with main (b8c811db).
Merge risk: high. The command paths now read every owner. The reconcile path that pull and uninstall use still has gaps in how it updates the other owners.
@jeff-r2026 your call on this one. This round shows the same pattern as the last: each fix to how the command path updates other scopes' records needs a matching change in reconcile. One function that both paths call to update the shared owner set would close this class instead of each case.
|
Addressed the latest review in 9664282 ( Findings addressed
Verification
Docker package SHA-256: Verification boundary: these are isolated Docker simulations with real agent packages and a mock HTTP backend, not live external MCP/LLM calls. Pull, init, binding and full uninstall use the installed CLI. Command report/sync/ack uses an awaited bundle of the production Please re-review this head; the successful local checks are not a claim that GitHub review/CI has approved it. |
Earlier findings: The findings from the last pass are resolved, and previously resolved findings remain resolved. These are distinct remaining reconciliation paths. Testing: The description documents representative real-client end-to-end verification, so no missing-E2E finding. I read the specified diff and its context without running, building, or installing PR code. |
Re-reviewed at
Nothing blocking is left from my side. Four non-blocking points:
The real-CLI record in the body is for an earlier head. CI has not run at this head yet. Merge risk: high. It writes into three tools' user configs that every workspace shares. The ownership model now has one function for both paths, which is the shape that was missing. @jeff-r2026 your call on this one. From our side the blocking items are fixed. What is left is the four points above, and whether the "one unreadable record freezes the target" behaviour is what you want. |
|
Fixed the two remaining reconciliation P1s in 70e6402, pushed to this PR.
Scope: the two P1s only; non-blocking P2 suggestions are unchanged. HTTP-mode reconcile skips delivery in this codebase, so the pull regression uses delivery-enabled git mode without matching history rather than claiming an HTTP delivery scenario. Verification: typecheck, changed-file type-aware oxlint and build passed. Affected compatibility regressions: 21 files / 2117 tests passed, with retries disabled (MCP suites, uninstall, local-agent model config and tool channels). Real-CLI limitation: attempted an isolated verify world (build 9664282+dirty, package sha256 prefix 9503743b8d90). init/pull/uninstall executed, but OpenClaw was not installed in that world and pull delivered nothing. This is NOT passing E2E evidence. Ledger reported 3 leaks (checkout snapshot changes while the world was running and project/.openclaw remaining); world cleaned afterward. Earlier-head Docker evidence is not claimed to verify this change. |
Read
One design point, not a defect in this diff:
The four non-blocking points from my last comment are unchanged, as the author says. The body notes there is no passing real-CLI run at this head: the world had no OpenClaw installed. The Docker record is from an earlier head. CI has not run here. @jeff-r2026 your call on this one. The blocking items from both reviews are fixed in the code. What is left is the four non-blocking points, a real-CLI run at the final head, and which scope wins when two deliver different values for one shared entry. |
|
No new findings confidently established in the specified diff. Resolved: Pull now consults matching external MCP ownership when updating shared entries, and previous-config cleanup protects surviving scope owners. The earlier resolved findings remain resolved. Testing: The PR description documents representative real-client end-to-end verification, satisfying the required testing record. No full tool × provider matrix is required. I only read the specified diff and its context; I did not run, build, or install PR code. |
|
Final-head Docker verification completed for 70e6402: 10 asserted scenarios passed using the freshly built, packed and globally installed CLI. Real installed client packages in the image: For each of OpenClaw, Hermes and DSH, the installed
Hermes additional scenario: deliver to both scopes in default home; initialize a different HERMES_HOME and workspace full-uninstall using it; original config must remain byte-identical. User full-uninstall from original home then removes the server. Package SHA-256: Boundary: real installed CLI and real client packages, isolated homes and local Git repos via a sandbox Git URL rewrite; no external MCP/LLM calls or session-hook timing claims. This run uses git-mode delivery, not HTTP command sync. No host configuration mounted into the container. Verification container removed; logs and JSON results retained locally as This supplements (does not relabel) the earlier unsuccessful verify-world attempt. No production code changes in this verification run. |
…locks The entry and every record stay as before, but the command now fails and names the record that could not be read, so the member knows what to fix instead of reading success while the server is still delivered.
OpenClaw, Hermes and DSH keep every scope's servers in one user config, so an uninstall keeps an entry another scope still records. The preview listed it as removed; it now marks it as kept.
A failed MCP removal marked every MCP file unread, so uninstall kept the whole delivered/mcp folder with a warning that a file could not be read, instead of naming the server folder the readable file still starts. The data home is still kept for the retry. Restores the Tencent#1787 assertion.
To get this merged, I pushed four commits on top of
Tests. Each fix has a test that failed first. The 2a and 2b tests cover OpenClaw, Hermes and DSH in user and workspace scope. README Real-CLI run of the first fix, built from Before these commits, the last ack was Two behaviours are unchanged, as before:
I'll merge once CI is green at |
Earlier findings: The previously reported issues are resolved in the current diff. Testing: The description documents representative real-client end-to-end verification; no missing-E2E finding. I only read the specified diff and its context. I did not run, build, or install PR code. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving at a82050b0. All blocking items from both reviews are fixed, and the open review points are closed by the commits on top. The CI review at this head raises one nit: a DSH key teamai did not create can be rotated once its hash matches. That can follow on its own. Two behaviours stay as described: an unreadable record holds every change to that config, and two scopes that deliver different values for one name write over each other. I'll merge once CI is green.
Earlier findings: The other previously reported issues are resolved. No additional findings confidently established. Testing: The description documents representative real-client end-to-end verification, satisfying the required testing record. I only read the specified diff and its context; I did not run, build, or install PR code. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving at 66c710a9, which is a82050b0 with main merged in. #1810 touched src/uninstall.ts as well, and the merge had no conflicts. The CI review at this head repeats the one nit on DSH key rotation, which can follow on its own. I'll merge once CI is green.
Summary
OpenClaw, Hermes and DSH keep their MCP servers and models in the single config each tool boots from — they have no project-scope file. This delivers a workspace's MCP server into that user config while recording ownership against the workspace, applies delivered models to each tool's provider store, and makes a re-delivery at either scope idempotent. Two behaviours outside the three tools change too:
restoreMcpDocrestores a failed write's exact bytes for every JSON MCP tool, andparseYamlno longer warns about an unresolved!!jstag in any YAML it reads.Type of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpasses (includes the generated tool-support docs check)npx vitest run src/__tests__/local-agent-mcp.test.ts src/__tests__/local-agent-model-config.test.ts src/__tests__/mcp-reconcile.test.ts src/__tests__/locate-mcp.test.ts→ 4 files, 1341 tests passedManual end-to-end check against real clients (Docker,
node:24, realopenclaw@2026.11.2/hermes-agent@0.23.0/@deepseek-ai/dsh@0.2.2-rc.1), mock backend sending workspace-scopeinstall_mcpandapply_model_config:~/.openclaw/openclaw.json,~/.hermes/config.yaml,~/.dsh/cordis.patch.yml) and the model provider; nothing was written under the workspace directoryinstall_mcp(user)theninstall_mcp(workspace)for the same server both succeed and each scope keeps its record; uninstalling one scope leaves the entry while the other still records itteamai hooks listreportsdsh installed