[YellowSnnowmann]

Summary

• Stops deep-cloning the large Config at each heavyweight consumer inside build_session_agent_inner: the tool registry, the reflection hook, and the turn provider now share one Arc<Config> (base_config) — refcount bumps instead of full clones.
• Extracts the App-UI-Control / App-Automation features toggle (Enabling App UI Control / App Automation doesn't grant click and type; mutations need Full autonomy or a UI-less config flag #3762) into a small, testable resolve_tool_config helper.
• Adds unit tests pinning the two invariants: the tool config shares the base Arc when the toggle is off, and when it fires the ax_interact_mutations grant is scoped to the tool registry and never leaks into the base config.
• Config is immutable after construction, so this is behaviour-identical.

Problem

build_session_agent_inner deep-cloned the full Config (117 pub items plus ~20 sub-schema modules) once per heavyweight consumer — at the tool-registry build, the reflection hook, and the turn provider. That is the per-agent-construction cost called out in #5050 (## Related).

Scope note (important): the issue's headline "lazy tool surface" refactor is sized against a per-agent-roster premise that does not exist in this repo — the built-in multi-agent path (subagent delegation) already Arc-shares one AgentSession's tool registry, config, memory, and provider. So this PR delivers the issue's Fix 1 (the Config-clone reduction, which pays off regardless of roster shape) and defers the lazy tool-surface refactor and the SQLite pool until the embedding pattern is confirmed. See Deferred below.

Solution

• Introduce base_config: Arc<Config> once at the top of the build; feed it (as Arc::clone) to the tool registry, the reflection hook, and the crate_native_provider call.
• resolve_tool_config(&base_config, &enabled_tools) returns the base Arc unchanged in the common case; only when the toggle actually fires does it clone-and-grant once, keeping that grant scoped to the tool registry so the provider/reflection keep the ungranted base.
• This maps exactly onto the issue's Fix 1 acceptance criterion (the clones at :396 / :409 / :1170). The owned-Config consumers (ArchivistHook::with_config, integration_runtime_config) are not in that list and live in the turn/archivist subsystems; they are left for the deferred fuller pass.

Deferred (out of scope here, by design)

• Fix 3/4 — lazy tool descriptors + apply the DomainSet filter before construction. These are one coupled refactor (a clean filter-before-construct needs the descriptor infrastructure; the naive alternative would hard-code ~325 tool names). Gated on confirming OpenCompany's embedding pattern.
• Fix 2 — shared SQLite connection/pool. Only pays off for a per-agent-roster host, which is not present in-repo; deferred to avoid a speculative shared-Mutex deadlock surface.

Submission Checklist

• Tests added or updated — tool_config_shares_base_arc_when_ui_control_toggle_off, tool_config_grant_is_scoped_and_leaves_base_untouched, tool_config_reuses_base_when_mutations_already_granted_globally; existing build_session_agent_* tests continue to pass (regression).
• Diff coverage ≥ 80% — the changed lines are the resolve_tool_config helper (fully covered by the three unit tests) and the Arc::clone sites (exercised by the existing build-path tests).
• Coverage matrix updated — N/A: internal performance refactor, no behaviour/feature change.
• All affected feature IDs listed — N/A.
• No new external network dependencies — none.
• Manual smoke checklist updated — N/A: behaviour-identical.
• Linked issue closed via Closes #NNN — N/A: delivers Fix 1 only; Fix 2/3/4 deferred (see Deferred).

Impact

• Performance: removes 3+ deep Config clones per agent construction (more when reflection is enabled), replacing them with refcount bumps. No behaviour change — Config is immutable post-build and the App-UI-Control grant stays scoped.
• Compatibility: none affected.

Related

• Delivers Fix 1 of Stop building the full tool surface and cloning Config on every agent construction #5050 (Config-clone reduction). Fix 2/3/4 deferred pending the OpenCompany embedding-pattern decision.
• Contributes to Keep embedded OpenHuman within a 20–30 MiB RAM budget for OpenCompany #5046 (embedded per-agent cost).

Changes since initial submission (review-driven)

• Logging on the new flow (62859aadd, CodeRabbit) — resolve_tool_config now logs an entry trace plus an exit debug line on both branches (granted vs reused-base), per the repo's grep-friendly-diagnostics guideline. CodeRabbit subsequently approved the PR.

---

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

• Key: N/A
• URL: N/A

Commit & Branch

• Branch: feat/lazy-tool-surface-5050

[coderabbitai]

📝 Walkthrough

Walkthrough

The session builder now shares Arc<Config> instances across components and uses resolve_tool_config to clone configuration only when tool-specific mutation access must be granted. New tests verify pointer sharing, scoped cloning, and preservation of the base configuration.

Changes

Shared configuration resolution

Layer / File(s)	Summary
Tool configuration resolution src/openhuman/agent/harness/session/builder/factory.rs, src/openhuman/agent/harness/session/builder/builder_tests.rs	resolve_tool_config conditionally grants computer_control.ax_interact_mutations, while tests verify shared or cloned Arc<Config> behavior.
Session builder configuration wiring src/openhuman/agent/harness/session/builder/factory.rs	Tool registries, reflection hooks, and the crate-native provider now reuse the shared base configuration where applicable.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• Issue 5050 — Covers the same factory.rs configuration-sharing and tool-registry cloning changes.

Possibly related PRs

• tinyhumansai/openhuman#4591 — Updates tool registry wiring to construct a tool with the resolved Arc<Config>.

Suggested labels: agent, feature

Suggested reviewers: m3ga-mind

Poem

I’m a rabbit with configs in a row,
Sharing their arcs wherever they go.
When tools need a grant, one clone takes flight,
The base stays safe, unchanged and right.
Tests hop along: pointer-equal delight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: sharing one Arc across tool, provider, and reflection paths for performance.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/openhuman/agent/harness/session/builder/factory.rs`:
- Around line 1230-1241: The base-config resolution flow needs diagnostics for
the no-op path as well as the grant branch. Update the resolver around the
ax_interact_mutations check to log grep-friendly entry, reuse-base branch, and
exit diagnostics, while preserving the existing grant behavior and its log.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9291ba3c-b27e-4161-8be9-b855d5e77b56

📥 Commits

Reviewing files that changed from the base of the PR and between d13a559 and dc5d766.

📒 Files selected for processing (2)

• src/openhuman/agent/harness/session/builder/builder_tests.rs
• src/openhuman/agent/harness/session/builder/factory.rs

[CodeGhost21]

LGTM — clean, well-scoped perf refactor.

Shares one Arc<Config> across the tool registry, reflection hook, and turn provider instead of deep-cloning the large Config at each site. Verified:

• Behavior-identical: resolve_tool_config scopes the App-UI-Control grant to the tool registry only; provider + reflection stay on the ungranted base — matching the old adjusted_config semantics. The scoping invariant is directly test-pinned.
• Type-checks: Arc::clone(&tool_config) for the Arc<Config> param and &tool_config deref-coerces to &Config for root_config (all_tools_with_runtime).
• Clone reduction holds in both branches (toggle-off 3→1, toggle-on ~3–4→2); Config immutability premise is sound.
• Tests cover all three helper branches with Arc::ptr_eq — the right assertion for a refcount-sharing refactor.
• Logging follows the grep-friendly-diagnostics convention.

Non-blocking: the single residual config.clone() into base_config could be eliminated by threading ownership from the caller in the deferred pass; a // TODO(#5050) breadcrumb would help. Deferral of Fix 2/3/4 is well-argued.

[CodeGhost21]

Review — approve ✅

Focused, well-tested performance refactor. Verified the diff against the pre-PR code.

Correctness — solid

• Behaviour-identical holds. Config is immutable post-construction, so N clones vs. one refcounted Arc are observationally equivalent. The provider and reflection hook previously received the ungranted config.clone(), and they still receive the ungranted base_config — the App-UI-Control grant stays scoped to the tool registry.
• The scoping invariant is the security-relevant one, and tool_config_grant_is_scoped_and_leaves_base_untouched pins it directly (asserts the base stays ungranted after a grant). That's exactly the failure mode that would matter — a mutation grant leaking into the provider/reflection config.
• Deref coercion is fine. tool_config is now Arc<Config>; &tool_config → &Config and &tool_config.browser etc. auto-deref through the Arc.
• Test visibility is correct — resolve_tool_config is pub(super), reachable from the descendant builder_tests module.

Conventions

• Logging follows the grep-friendly-diagnostics rule well (action=resolve_tool_config phase=enter/exit outcome=... with correlation fields). Entry trace + branch-specific exit debug is the right granularity.
• Test naming + inline rationale clearly state the invariant each protects.

Minor (non-blocking)

• The "removes 3+ deep clones" framing is optimistic for the common path. No reflection + no toggle: old = 2 clones (tool registry + provider), new = 1 (base_config). The 3+ figure only holds with reflection enabled (old 3 → new 1) or when the toggle fires. Direction is right and the win is real — just worth tightening the description's floor.
• One unavoidable clone remains at Arc::new(config.clone()) since config arrives as a borrow. Eliminating it means threading Arc<Config> in from the caller — reasonably deferred alongside the owned-config consumers you already flag.

Risk

Low. No behaviour change, no new deps, no public API change. The only conceivable regression (grant leaking beyond the tool registry) is explicitly tested. Deferred Fix 2/3/4 are clearly scoped out.