Go2 agentic architecture layers and self-evolution review#2727
Open
z1050297972-spec wants to merge 41 commits into
Open
Go2 agentic architecture layers and self-evolution review#2727z1050297972-spec wants to merge 41 commits into
z1050297972-spec wants to merge 41 commits into
Conversation
z1050297972-spec
requested review from
leshy,
paul-nechifor and
spomichter
as code owners
Contributor
Greptile SummaryThis draft PR introduces a Go2 agentic architecture extension (Layers 3–6) covering agent brain, world state, skill interfaces, and robot body, along with a local evolution ledger for reviewable self-evolution artifacts. It also carries several upstream fixes: async MCP client initialization, direct RPC tool dispatch, task-scoped outcome recording, JS-based Rerun iframe URL construction, and MuJoCo headless mode.
Confidence Score: 4/5Safe to merge after the default model value in McpClientConfig is restored or made explicit-only; the rest of the changes are well-tested and the previous-thread bugs have been addressed. The one issue that could affect every existing McpClient user is the default model switch: any deployment that omits an explicit model setting now silently targets a local Ollama server instead of gpt-4o, and the new async init loop will retry indefinitely without surfacing the misconfiguration. All other concerns — proposal collision, InterventionLog clear-then-load, outcome task isolation, dashboard localhost hardcoding — were resolved in earlier review rounds and the fixes are present in this diff. dimos/agents/mcp/mcp_client.py — the McpClientConfig.model default change is the one item that needs a decision before merging. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant McpClient
participant InitThread as Init Thread
participant McpServer
participant SkillOutcomeStore
participant CausalWorldModel
User->>McpClient: on_system_modules(modules)
McpClient->>InitThread: spawn _delayed_register_and_initialize_agent_graph
InitThread->>InitThread: _register_direct_tools (builds _direct_rpc_calls)
InitThread->>McpServer: _try_fetch_tools (HTTP)
McpServer-->>InitThread: tools/list
InitThread->>McpClient: "_state_graph = create_agent(model, tools)"
InitThread->>McpClient: start _thread_loop
User->>McpClient: human_input(go to kitchen)
McpClient->>McpClient: "_last_user_task = go to kitchen"
McpClient->>McpClient: queue HumanMessage
McpClient->>McpServer: _mcp_tool_call(navigate_with_text, args)
Note over McpClient,McpServer: Direct RPC path if tool in _direct_rpc_calls
McpServer-->>McpClient: result
McpClient->>SkillOutcomeStore: "record_skill_outcome(task=go to kitchen, ...)"
McpClient->>CausalWorldModel: "record_causal_transition(task=..., skill_name=...)"
CausalWorldModel->>SkillOutcomeStore: "get_recent_outcomes(skill_name=..., task=...)"
SkillOutcomeStore-->>CausalWorldModel: filtered outcomes
CausalWorldModel-->>McpClient: transition recorded
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant McpClient
participant InitThread as Init Thread
participant McpServer
participant SkillOutcomeStore
participant CausalWorldModel
User->>McpClient: on_system_modules(modules)
McpClient->>InitThread: spawn _delayed_register_and_initialize_agent_graph
InitThread->>InitThread: _register_direct_tools (builds _direct_rpc_calls)
InitThread->>McpServer: _try_fetch_tools (HTTP)
McpServer-->>InitThread: tools/list
InitThread->>McpClient: "_state_graph = create_agent(model, tools)"
InitThread->>McpClient: start _thread_loop
User->>McpClient: human_input(go to kitchen)
McpClient->>McpClient: "_last_user_task = go to kitchen"
McpClient->>McpClient: queue HumanMessage
McpClient->>McpServer: _mcp_tool_call(navigate_with_text, args)
Note over McpClient,McpServer: Direct RPC path if tool in _direct_rpc_calls
McpServer-->>McpClient: result
McpClient->>SkillOutcomeStore: "record_skill_outcome(task=go to kitchen, ...)"
McpClient->>CausalWorldModel: "record_causal_transition(task=..., skill_name=...)"
CausalWorldModel->>SkillOutcomeStore: "get_recent_outcomes(skill_name=..., task=...)"
SkillOutcomeStore-->>CausalWorldModel: filtered outcomes
CausalWorldModel-->>McpClient: transition recorded
Reviews (11): Last reviewed commit: "Merge branch 'main' into refactor/go2-ar..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
write_skill_proposal_file derived the artifact path solely from proposal_id via _safe_filename, then atomically replaced it. A reused stable proposal_id (e.g. improve_navigation) would silently overwrite the earlier review artifact, losing the proposal reviewers were meant to inspect. Mirror the events path's _unique_event_path approach: append -1, -2, ... when the path exists so both artifacts are kept. The author's proposal_id stays verbatim in the JSON; only the filename is disambiguated.
_safe_filename maps every non-alphanumeric character to '_', so
distinct proposal_ids such as skill-v2 and skill.v2 both reduce to
skill_v2 and land on the same path. The prior -N suffix guard only
treated genuine same-id reuses; it would file the second distinct
proposal as skill_v2-1.json, conflating unrelated proposals as
iterations and risking overwrite of the first when ordering changes.
Anchor the stem on an injective digest of the original (un-sanitized)
proposal_id: {slug}-{sha1(id)[:8]}. Distinct ids now always map to
distinct files; the -1/-2 suffix still handles true same-id reuse.
JSON proposal_id stays verbatim. This mirrors how the events path uses
a timestamp prefix to sidestep lossy sanitization.
| payload = json.dumps(normalized, indent=2, sort_keys=True) + "\n" | ||
| tmp_path = path.with_name(f".{path.name}.tmp-{os.getpid()}") | ||
| tmp_path.write_text(payload, encoding="utf-8") | ||
| tmp_path.replace(path) |
Contributor
There was a problem hiding this comment.
_unique_proposal_path() before this replace(), but the final write still overwrites if another agent creates the same filename between the exists() check and the replace. Two concurrent calls with the same stable proposal_id can both choose the base file, both return success, and only the later proposal remains in the ledger. The suffix selection needs to claim the final filename atomically, such as with an exclusive create or a lock around path selection and write.
…ct RPC routing Direct tool registration was routing calls to skill.class_name/skill.func_name instead of the module's actual RPCClient.remote_name. When workers register with aliased or remapped names the direct path sends tool calls to a route that does not match the running module, causing failures or empty results.
… hardcoding localhost The iframe src was hardcoded to localhost:9090 and localhost:9876, ignoring DIMOS_RERUN_HOST / --rerun-host and listen_host. When the dashboard is served from a remote machine the embedded Rerun viewer connects to the browser user's localhost and the stream never appears. Read the template at serve time, substitute the correct host and ports (RERUN_WEB_VIEWER_PORT=9878, RERUN_GRPC_PORT=9877).
write_text() directly on the shared JSON state file is not atomic: - a crash during write truncates the file - concurrent agent instances interleave writes, losing transitions Write to a temp file in the same directory, then os.replace() for an atomic swap that guarantees readers always see a complete file.
_load_world_model_state_from_path and InterventionLog.load_dict both cleared in-memory state (transitions, records) before parsing the payload. If any item failed to parse the running model was left corrupted. Build all new objects in temporaries first, swap in only after all parsing succeeds. A bad schema, malformed transition, or float() conversion failure no longer destroys the running model's state.
…k evidence leakage get_recent_outcomes() filtered only by skill_name, so unrelated tasks shared outcome history. When the causal world model recorded a transition without an explicit outcome_json it fell back to _latest_outcome(skill_name), which could grab another task's navigate_with_text success/failure and feed it as evidence for the current task. Add a 'task' dimension to SkillOutcome, record_skill_outcome, and get_recent_outcomes. _latest_outcome now filters by both skill_name and task so that 'go to kitchen' outcomes never leak into 'go to office' predictions.
Auto-recorded outcomes from McpClient._record_tool_outcome were missing the 'task' field, so they were stored with task="". Later, _latest_outcome(skill_name, task="go to the kitchen") filtered by the non-empty task and never matched, causing the causal world model to record transitions with outcome_success=None (unknown). Capture the last user input as _last_user_task and inject it into both _outcome_payload_from_result and _record_tool_exception so auto-recorded outcomes carry the same task key that _latest_outcome filters by.
…nd address The server-side resolved host (listen_host=127.0.0.1 or rerun_host) was used as the iframe target. When the browser is on a different machine: - 127.0.0.1 → browser tries to reach its own loopback - 0.0.0.0 → not a routable address Use window.location.hostname in JavaScript to build the iframe URL client-side, so the browser always connects to the same host it used to load the dashboard page. Ports (9877 gRPC, 9878 web viewer) are hardcoded constants matching the Rerun bridge defaults.
Contributor
|
Thanks for sharing! Much of this including spatial memory and skill interfaces are already implemented. We're working on new agent features somewhat along these lines now, woudl reccomend expanding / double clicking on that area and exactly the set of requirements that would be interesting, and what is the end use case you're trying to achieve. Send the desired prompts on your wishlist that don't work today. For example "hey robot go sit down to the left side of the couch over there" |
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi DimOS maintainers,
I am opening this as a draft PR for architecture review and discussion. The goal is to share my current thinking around a Go2 agentic architecture extension, especially around context scheduling, skill-interface evolution, and reviewable self-evolution artifacts. I am not asking for an immediate merge before the design direction is reviewed.
Architecture overview
The main review figure is below. It combines the repo-wide DimOS runtime architecture with the Go2 agentic extension in one diagram, so the agent layer can be reviewed in the context of blueprints, runtime modules, transports, skills, memory, and robot targets.
Editable SVG source:
https://github.com/z1050297972-spec/dimos/blob/refactor/go2-architecture-layers/docs/reviews/2026-07-05-dimos-agentic-architecture.svg
PNG render in the branch:
https://github.com/z1050297972-spec/dimos/blob/refactor/go2-architecture-layers/docs/reviews/2026-07-05-dimos-agentic-architecture.png
Detailed review notes:
What this branch explores
This branch proposes a more explicit Go2 agentic layering model:
The important design boundary is that self-evolution is represented as reviewable artifacts, not automatic code mutation. The agent can record events and proposals, but those artifacts still need human or downstream tooling review before becoming implementation changes.
Main technical changes
unitree_go2_agenticblueprint does not wireWebsocketVisModuledirectly.maininto this branch and preserved upstream infrastructure changes while keeping the local Go2 layered architecture work.Review questions
I would especially appreciate feedback on these points:
ContextProviderboundary the right place to decide which memory/context is effective, with the LLM remaining the main judgment owner after context is selected?Validation performed
mainat6e813a72and resolved merge conflicts while preserving upstream infrastructure updates.git diff --check/git diff --cached --checkon the changed docs, SVGs, and changelog updates during the documentation work.MUJOCO_LOG.TXT.Notes
This is intentionally opened as a draft PR. I expect some parts may need to be split, renamed, or moved to more general DimOS agent packages after maintainers review the architecture direction.