Repository navigation
refactor(skills,mcp): move skills and MCP machinery onto tinyskills and tinymcp (#2576) - #2580
Conversation
…humansai#2576) scrub, redact and SCRUB_MAX_BYTES are used by Composio, search, the turn settle path and the OAuth callback, not only MCP. Moving them to an ungated crate::redact lets the MCP code move without dragging every non-MCP caller along, and runs their tests in the default lane. Bodies and tests move verbatim.
…rate::mcp (tinyhumansai#2576) company/mcp*.rs become mcp/decl/ (declarations, mcp.json, endpoint, server info, families) and mcp/policy/. Rename only: both stay ungated so their tests keep running in the default lane, and company::mcp stays addressable through a re-export.
…inyhumansai#2576) decl/mod.rs was 1005 lines, over the source cap. Storage and resolution move to decl/store.rs, validation and normalization to decl/validate.rs, re-exported so every existing path still resolves. No code changes.
Rename only. The probe, failure classifier and McpFailureQueue keep their bodies; callers are re-pointed from harness::mcp_probe.
tinyhumansai#2576) harness/built_in/mcp.rs splits along its existing seam: the per-agent exposure (attachments, briefs, registry tools) becomes mcp/agent/, the company-scoped registry store becomes mcp/runtime.rs, each with its own tests. Rename only; callers are re-pointed from harness::mcp.
…yhumansai#2576) resolve_for_agent folds the company's declarations, the agent's effective grants and the registry store into an AgentMcp that answers which servers ride on the spec, which registry tools go on the belt and what the persona brief says. build_agent_with_model now asks it instead of carrying three separate blocks. Inputs and outputs are unchanged: a dump of native names, tool scope, shared belt, attachments and system prompt across ten grant/server shapes is byte-identical before and after.
OPENHUMAN_NATIVE_TOOLS names mcp_list_tools and mcp_call_tool, and the belt drops those names before it reaches the runtime, so the McpListToolsTool and OcMcpCallTool pushed for a declared-server agent never ran: every call went through OpenHuman's own mcp_call_tool over the servers attached to the spec, whose deny list carries the per-agent blocks. Removed with it: registry_for_agent, granted_secrets, granted_policies, McpMetering, required_string_arg and McpToolPolicySet, none of which had another caller. The two bridge names now come from AgentMcp::native_tool_names under the same condition registry_for_agent used, so the ToolScopeSpec each agent is registered with is unchanged; the ten-shape scope dump is byte-identical before and after. Credential-reflection canaries move onto the native mcp_call_tool, the path agents actually take. Per-call failure surfacing and OauthCall metering were never live on this path and return with the tinymcp call-outcome observer.
…nsai#2576) mcp_list_tools and mcp_call_tool are in an agent's native scope only when an enabled server its grants name is declared, and never on its belt; a wildcard grant attaches every enabled server and a named one only its own.
…ai#2576) Drives the real pool and hosted provider against a scripted model and a loopback MCP server. With the writer blocked from one tool, its native mcp_call_tool is refused and the server sees no call, another tool on the same server succeeds, and a teammate the rule does not name still reaches the blocked tool. Clearing the block moves the MCP fingerprint and the writer's next call succeeds on the same pool.
…#2576) The mcp feature lane filtered to build, app::types and hive tests, so the mcp-gated tests under crate::mcp were compiled by the all-features check and run by nothing.
tinyhumansai#2576) A golden snapshot of all 111 SKILL.md files under companies/ (108 bundle skills plus the 3 global baseline skills): the digest of render(parse(src)), the source digest a registry install pins, and the digest of the frontmatter lines the parser keeps aside. Generated on main 7af9e56 so the parser and digest swaps that follow cannot shift an existing install's pin unnoticed. Re-bless with BLESS_SKILL_PINS=1.
…pe (tinyhumansai#2576) The per-agent skill narrowing lived in runtime::builder, so the company layer's skill_effective and skill_scope called up into the runtime to reach it. It now lives beside the per-skill inversion it mirrors, with its tests; behaviour is unchanged.
…mansai#2576) skill_effective::resolve read the global baseline from crate::globals itself and joined skills/ onto a company source dir. It now takes a SkillLayers { baseline, bundle_root, library } and reads nothing ambient, so the same fold can serve an owner whose layers differ. company::skill_set names a company's layers (global baseline, the bundle under its source dir, the shared library) in one place; every reader goes through resolve_company / resolve_company_for_agent. Output unchanged.
…nyhumansai#2576) Five readers (the REST list, slug collision check, document reader, GraphQL Company.skills and the teammate picker) each listed the deltas, appended the [globals].disable synthesized disables, fetched the shared library and resolved. They now call load_runtime_skill_set; the harness reads its deltas through load_skill_deltas, keeping its own disable source (the manifest) and the delta fingerprint over the same rows. SkillOwner names whose deltas are read so an agent key can join it later. The reader-agreement test now drives each loader against a real delta store and checks REST, GraphQL, the picker, the document reader and the materialized tree report one set.
…inyhumansai#2576) skill_md assembled a document by hand with a second copy of the renderer's one-line collapse. It now builds a SkillDoc (no version, body plus the trailing newline it always added) and renders it; a test pins the output byte for byte against the old assembly.
…#2576) The shared library was a skills_root path plus a OnceLock cache on AppState, set only by serve from its --company checkout. It is now a company::skill_library::SkillLibrary: DirLibrary (the same load, cache and Config-error recast) or NoLibrary, picked by for_host in the order explicit dir, OPENCOMPANY_SKILL_LIBRARY, packaged copy, none. A named directory that is missing still fails; a packaged copy is served only when present. AppState holds the library and shared_skill_registry() delegates to it. RuntimeBuilder::with_skills_registry takes the library and snapshots it into the harness at build. serve, the desktop builder and provisioning call checked_skill_library(), so a broken library still fails boot or 500s a provision. The desktop host now resolves the library too (env only until the packaged copy is wired), so the parity exemption for state.with_skills_root is gone.
The desktop served no shared skill library: a packaged install has no checkout, so the registry tab was empty and every install fell back to a client-authored stub. The bundle now carries every company's skills/ as Tauri resources (layout preserved, read from disk, not compiled in), the shell resolves the resource directory before it starts its hosts, and each embedded host serves that copy unless OPENCOMPANY_SKILL_LIBRARY names another. With a library present, installing a slug the library does not serve is a 404 rather than a stub. Tests boot an embedded host over the packaged layout and check the registry lists it, an install snapshots the library's document, an unknown slug 404s, and the bundle's resource pattern lands where the host looks.
…inyhumansai#2576) The image sets OPENCOMPANY_SKILL_LIBRARY=/app/companies so a tenant provisioned with no company still serves the library it ships, and desktop-dev.sh points the shell at the checkout's companies/ so local edits are what it lists.
…nyhumansai#2576) Probes OpenHuman's read_skill_resource across in-place re-materializations of one agent's skill tree: an edited resource is read fresh and list/describe see a newly added skill, but read_skill_resource resolves skills through OpenHuman's process-wide metadata cache, which nothing invalidates for this tree, so an added skill's resources are not found until restart and a removed one fails on its missing directory. Recorded as-is for an upstream fix; not patched here.
…yhumansai#2576) The shared library section names the resolution order (explicit, OPENCOMPANY_SKILL_LIBRARY, packaged, none) and what fails boot; install case 3 now applies only to a host naming no library. The fold's explicit layers, the skill_set loaders, the moved scope derivation, the golden pin snapshot and the read_skill_resource stale-scan gap are recorded.
The bundle's mcp.json is read in the default lane, and the parser moves onto tinymcp's config_doc. Same vendored path as before, so the lockfile is unchanged.
…ai#2576) The bundle file is now parsed by config_doc::parse_with in lenient mode, with endpoint, readOnlyTools, authSecret and $comment registered as host fields. OC keeps only what is particular to a committed bundle: the endpoint spelling, the shared validator, and refusing a credential in the query string or an inline headers block. An unknown entry field now drops that entry and is reported, where serde's deny_unknown_fields used to refuse the whole file.
…nsai#2576) OcMcpRegistryScopedTool now attaches tinymcp's McpCallOutcome to every mcp_registry_tool_call result: answered when the install replied, failed with the error's wire code when the vendored tool reports a string error body, and ToolNotAllowed / InvalidArguments on its own refusals. The text classifier is pinned against tinymcp's own error renderings.
…ansai#2576) mcp::observe reads the McpCallOutcome that tinymcp's mcp_call_tool (and the registry call tool) attach to their results, as OpenHuman forwards it on ToolCallCompleted / SubagentToolCallCompleted. An answered call records one OauthCall sample, as the removed OcMcpCallTool did. A call that failed before the server answered becomes a scrubbed McpFailure, classified by the new probe::classify_call_error; refusals (ToolNotAllowed, bad arguments) are left to the agent's result, as before. Driven by real bridge results against loopback servers: answered, remote isError, blocked, 401 with and without a credential or OAuth challenge, unreachable, JSON-RPC error, reflected-secret scrubbing.
…humansai#2576) HarnessDeps.mcp_failures is now the company's McpCallObserver. Each agent's blueprint carries an AgentMcpObserver bound to its company, id, meter and the declared servers; CompanyAgent observes the progress stream after every turn, and the hive settle path does the same for coordinator turns. The brain and settle drains, the clear-before-turn calls, the bubble steps and the McpCallFailed journal path are unchanged. McpFailureQueue is deleted; nothing had pushed onto it since OcMcpCallTool was removed. The confined copilot agent gets an inert observer.
…inyhumansai#2576) A live harness turn against a loopback MCP server meters the answered call as one OauthCall and records nothing; a call to a server answering 401 lands on deps.mcp_failures as credential_required and is not metered. The registry-scoped tool's answered, refused and failed calls read the same way through the observer.
…ansai#2576) Discovery, dynamic client registration, PKCE, the parked pending state, the code exchange and the refresh now come from tinymcp's OAuthFlow with require_public_endpoints on, so every refresh re-applies the endpoint guard (OAuthFlow::refresh, not the free refresh_if_expired). The guard is tinymcp's, a superset of the ranges OC refused, and pins the client to the addresses it checked. OC keeps the host side: the company/server id a redirect resolves back to, and MaterialStore, which maps the flow's credential map onto the stored AuthMaterial::OAuth so existing tokens keep working and the callback still stores through store_auth. AppState holds the flow instead of its own pending map. Routes, status codes and error variants are unchanged; supports_console_oauth keeps its own discovery because OAuthFlow::detect reads a discovery failure as a static token, where the probe must leave the Sign in button alone. Deleted: OC's PKCE, DCR, authorize-URL builder, token exchange and parse, SSRF guard, refresh, needs_refresh, PendingOAuth/OAuthBegin and the AppState park/take/TTL code.
…humansai#2576) skill_digest was the same lowercase-hex SHA-256 of one rendered document that tinyskills now exports as document_digest. Call sites use it directly; the OC function and its known-answer tests are deleted.
…inyhumansai#2576) slugify, validate_slug, validate_slug_shape and valid_slug now run on tinyskills SlugRules (Separator punctuation, truncation, reserved routes, `skill` fallback), and the frontmatter-size and description-length checks on check_frontmatter_size / validate_description_chars. OC keeps its operator-facing sentences; tests pin them and compare slugify with the previous derivation.
…ai#2576) company::skill_scan was the scanner and catalogue sanitizer tinyskills now owns, check for check. The write plane scans SkillDoc::scan_document and the harness renders catalogue text with tinyskills' sanitize_catalogue_text; the OC module and its duplicated tests are deleted. The shipped-bundles-scan-clean test stays, beside the other shipped-bundle tests.
…archive (tinyhumansai#2576) skill_upload's zip reader (entry and byte ceilings, path, link and nesting checks, macOS metadata, single-root and SKILL.md lookup) was the reader tinyskills now ships behind its `archive` feature, which opencompany-core enables. OC maps each ArchiveError to the sentence the upload dialog showed before and still refuses bundled extras by name. Entries prefixed `./` are now read as the same paths, which is the accepted upload behaviour; a test pins it. The desktop lockfile also gains the tinyskills edge the vendor pin commit left out.
…humansai#2576) EffectiveSkills::materialize hands its enabled entries to tinyskills::materialize_tree instead of clearing the scratch tree and copying bundles with its own copy_dir_recursive, which is deleted. tinyskills is stricter in three ways, each pinned by a test: a bundle nested more than 32 directories deep fails the materialization; a symlinked skills root is unlinked rather than followed; a bundle whose directory name is not a safe path segment is now left out of the tree and the catalogue (with a warning) instead of being copied.
… tinyskills v0.2.8 (tinyhumansai#2576)
…-based materialize_tree (tinyhumansai#2576)
|
Warning Review limit reached
This review includes 190 billable files and costs up to $47.50. View limit detailsReview configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (2)
📒 Files selected for processing (190)
Comment |
Tiny Sweeper review
|
|
@coderabbitai can you review this now? |
|
|
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Reviewed the full diff against the vendored tinymcp v0.4.0 and tinyskills v0.2.8 sources at the PR's pins. The skills port looks faithful (limits, Mac-metadata handling, reserved-slug suffixing, BOM handling all match the deleted host code), and the scrubbing invariant the old OcMcpCallTool carried is now covered by tinymcp's SecretScrubber plus the observer on recorded failures. I also checked that a delegated turn's MCP calls are observed once (OpenHuman emits either ToolCallCompleted or SubagentToolCallCompleted, never both).
Requesting changes for four correctness issues in the console OAuth swap and the new call observer, inline below. The other five comments (error-prose coupling, two efficiency items, a dead helper, missing per-directory READMEs) are take-or-leave.
Fix before merge
begin_errorreports a transient protected-resource metadata fetch failure as "does not advertise DCR, paste a token" (tinymcp usesAuthDiscoveryfor both).- The observer classifies every registry-install 401 as credential-required because
auth_configuredis looked up by declared name and registry outcomes carry aserver_id. - A server name containing
/breaks the callback's company lookup now that company and server share one slash-separated id. - A replayed or raced callback renders a 502 "token exchange failed" instead of the 400 "Sign-in expired" page.
| || v6.is_multicast() | ||
| || (v6.segments()[0] & 0xfe00) == 0xfc00 // unique-local fc00::/7 | ||
| || (v6.segments()[0] & 0xffc0) == 0xfe80 // link-local fe80::/10 | ||
| tinymcp::Error::AuthDiscovery { .. } => OpenCompanyError::InvalidRequest(format!( |
There was a problem hiding this comment.
Transient discovery failure reported as "no DCR". tinymcp's transport also returns Error::AuthDiscovery when fetching the protected-resource metadata fails (transport/http/mod.rs wraps the fetch error as AuthDiscovery { detail: "fetching protected-resource metadata: …" }), and OAuthFlow::begin propagates it with ?. This arm turns that into the 400 "requires OAuth but does not advertise dynamic client registration — paste a static API token".
Scenario: the server's /.well-known/oauth-protected-resource briefly returns 503; the operator is told the server lacks DCR and to paste a token, for a server that signs in fine a minute later. Before the swap this was Harness("oauth discovery failed: …").
Suggest matching on the detail prefix ("fetching protected-resource metadata" → Harness) so only the genuine no-usable-authorization-server case gets this message, or have tinymcp return Transport/Http for the fetch.
There was a problem hiding this comment.
Fixed in 52d4240: an AuthDiscovery whose detail starts with fetching protected-resource metadata now maps to Harness("oauth discovery failed: ..."); only the no-usable-authorization-server case keeps the "paste a token" message. Regression test serves a 503 on the metadata route.
| registration — paste a static API token in its credential field instead." | ||
| )), | ||
| tinymcp::Error::MalformedResponse { detail } | ||
| if detail.contains("endpoint refused") |
There was a problem hiding this comment.
Status code keyed on tinymcp's message prose. The 400-vs-500 decision here depends on substrings of MalformedResponse.detail ("does not require authorization", "endpoint refused", "endpoint has no host", starts_with("invalid ")). A reworded message in a later tinymcp bump compiles fine and silently turns an actionable 400 into a 500 "oauth sign-in could not start".
Since tinymcp is vendored and in-house, the right-depth fix is typed variants (e.g. Error::NoAuthorizationRequired, Error::EndpointRefused) matched here instead of prose. Non-blocking.
There was a problem hiding this comment.
Partly: tinymcp v0.4.0 has no typed variant for these (flow.rs and endpoint_guard.rs build them with Error::malformed(..), so MalformedResponse { detail } is all there is), so the arms still match prose. Fixed in be92b1a by naming each fragment as a constant and adding a test that include_str!s the tinymcp sources and fails if any fragment stops appearing there, so a reword breaks CI rather than turning a 400 into a 500. Typed variants (NoAuthorizationRequired, EndpointRefused) would still be the right fix on the tinymcp side.
| /// scrubbed with every declared server's credentials, like the message. | ||
| fn failure(&self, outcome: &McpCallOutcome, output: &str) -> Option<McpFailure> { | ||
| let error = outcome.error.as_ref()?; | ||
| let decl = self.servers.iter().find(|decl| decl.name == outcome.server); |
There was a problem hiding this comment.
Registry-install 401 is always classified as credential-required. auth_configured is derived by matching outcome.server against the declared servers' names, but for a mcp_registry_tool_call outcome outcome.server is the registry server_id, which never matches a declared name, so this is always false for registry installs.
Scenario: a directory install with stored env credentials gets a 401 because its token was revoked. call_error_from_text → UNAUTHORIZED, classify_call_error → status_kind(401, false) → CredentialRequired, and the operator bubble says "MCP server '' needs a credential. Add its API token …" even though one is configured and the real state is token-rejected. Registry calls were not classified at all before this PR, so this is a new misleading note.
Suggest resolving configured-ness for registry ids through McpRuntime (the install's env-key names), or emitting no auth hint for registry outcomes.
There was a problem hiding this comment.
Fixed in ea9b84d: the observer now looks up configured-ness as Option<bool>; a server it cannot match to a declaration (a registry install id) gets None, and classify_call_error maps a 401 with unknown auth to the generic error class with no auth hint instead of credential_required. Regression test uses a registry-shaped 401 outcome; the existing registry observer test that encoded the old status is updated.
| (verifier, challenge) | ||
| /// The company and server a [`server_id`] names. | ||
| pub fn split_server_id(server_id: &str) -> Option<(CompanyId, String)> { | ||
| let (company, server) = server_id.rsplit_once(SERVER_ID_SEPARATOR)?; |
There was a problem hiding this comment.
A server name containing / breaks callback resolution. The pending authorization is parked under "{company}/{server_name}" and recovered with rsplit_once('/'), but validate_one does not forbid / in a server name.
Scenario: operator adds a runtime server named acme/docs (accepted), opens /mcp/servers/acme%2Fdocs/oauth/start (axum decodes the segment to acme/docs). server_id becomes cid/acme/docs; the callback's split_server_id yields company cid/acme, server docs; state.registry().get misses and the tab shows 404 "unknown company". Before the PR PendingOAuth carried company_id and server_name as separate fields, so this worked.
Either reject / in validate_one or pick a separator no company id or server name can contain.
There was a problem hiding this comment.
Fixed in 2722465: the company half of the server id is escaped (% and /) and the id is split on the first /, so any server name round-trips, including acme/docs. Test covers slashes and percent signs in both halves.
| let status = resp.status(); | ||
| let json: Value = resp.json().await.map_err(|e| { | ||
| OpenCompanyError::Harness(format!("client registration returned non-JSON: {e}")) | ||
| .map_err(|error| OpenCompanyError::Harness(format!("token request failed: {error}")))?; |
There was a problem hiding this comment.
Unknown/expired state is rendered as a 502 exchange failure. Every OAuthFlow::complete error, including tinymcp's MalformedResponse("unknown or expired oauth state"), becomes Harness("token request failed: …"), so the callback route renders the 502 "token exchange failed" page.
Scenario: the browser (or a double-click) delivers the same callback twice; both pass pending_server(cb_state), the first exchanges and consumes the entry, the second hits this arm and the operator sees a 502 and a warn log even though the sign-in succeeded. take_oauth used to route this to the 400 "Sign-in expired" branch; map that specific error there.
There was a problem hiding this comment.
Fixed in 411cad1: complete maps tinymcp's unknown or expired oauth state to InvalidRequest, and the callback renders that as the 400 "Sign-in expired" page; other exchange errors stay a 502. The prose is pinned to the tinymcp source by the same coupling test as above. Tested at the response-mapping level (a raced callback itself needs two concurrent requests, so the mapping is what is asserted).
| company.clone(), | ||
| manifest_agent.id.clone(), | ||
| deps.meter.clone(), | ||
| deps.mcp_servers.clone(), |
There was a problem hiding this comment.
Every agent clones the whole credentialed decl list. deps.mcp_servers.clone() copies every declared server — resolved AuthMaterial, tool_policies, tool_inventory — into each AgentBlueprint's observer, where only the name, auth.is_configured() and secret_values() are read. N agents × M servers full copies per roster build and per MCP-freshness rebuild, each long-lived in a CompanyAgent; the old path kept one Vec<String> of granted secrets per agent.
Build one Arc<[McpServerDecl]> (or a slim (name, configured, secrets) list) once per HarnessDeps and share it. Non-blocking.
There was a problem hiding this comment.
Fixed in e216f8c: observers no longer hold the decls. The company's McpCallObserver keeps one slim (name, configured) + secrets snapshot and every agent built over unchanged servers gets the same Arc; a change in the servers replaces it. Test asserts sharing and replacement.
| .filter(|decl| decl.enabled && grants_cover_server(grants, &decl.name)) | ||
| .cloned() | ||
| .collect(); | ||
| let declared_wired = !granted.is_empty() && !registry_from_decls(&granted).is_empty(); |
There was a problem hiding this comment.
A full registry is built only to test is_empty(). registry_from_decls(&granted) assembles an oh::mcp::host::static_registry for every granted server and drops it; embed_servers() then re-runs the same grant filter over decls.
If the check is guarding against an unbuildable set, compute granted once, keep it on AgentMcp for embed_servers, and either reuse the registry or test buildability without constructing it. Non-blocking.
There was a problem hiding this comment.
Partly, in acd3c67: granted is computed once, kept on AgentMcp, and embed_servers reuses it, so the grant filter no longer runs twice and the decls are borrowed instead of cloned. I kept the is_empty() check on a built registry on purpose: static_registry degrades an unbuildable set (e.g. an invalid header-auth name) to empty, and that is the only cheap way to keep such a server from being wired; there is no buildability test short of constructing it.
|
|
||
| /// A company's effective skill set: the global baseline, the bundle under | ||
| /// `source_dir`, and `library`, folded with `deltas`. | ||
| pub fn resolve_company( |
There was a problem hiding this comment.
skill_set::resolve_company has no production caller. Every reader goes through load_runtime_skill_set, the harness through resolve_company_for_agent; the resolve_company GraphQL calls is server::graphql::skills::resolve_company, a different function. Two same-named resolve entry points with one dead is the drift this PR's "six copies collapse into one" aims to remove. Drop it or make resolve_company_for_agent delegate to it. Non-blocking.
There was a problem hiding this comment.
Fixed in 03d727a: resolve_company is #[cfg(test)] now (its only callers are tests); resolve_company_for_agent is the one production entry point.
| use crate::company::mcp::{AuthMaterial, McpServerDecl}; | ||
| use crate::runtime::tools::grants_cover_server; | ||
|
|
||
| mod registry_list; |
There was a problem hiding this comment.
No README.md in the new src/mcp/{agent,decl,policy}/ directories. CLAUDE.md ("Project Structure & Module Organization"): "Every source directory carries a README.md describing what lives there, file by file." src/mcp/README.md has the table, but the three subdirectories carry none; src/harness/built_in/hive_hooks/README.md is the precedent for a sibling subdirectory. Either add them or move the file-by-file rows down. Non-blocking.
There was a problem hiding this comment.
Fixed in 7631505: added README.md to mcp/decl, mcp/policy and mcp/agent with the file-by-file tables, and mcp/README.md now points at them.
…overy error, not missing DCR (tinyhumansai#2576)
… not a failed exchange (tinyhumansai#2576)
…uth state is unknown (tinyhumansai#2576)
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Re-reviewed at 7631505. All nine items from the first pass are addressed, each with a regression test, and I checked the fixes against the delta rather than the replies alone:
AuthDiscoveryfrom a failed metadata fetch is a 500 "discovery failed" again; only the no-usable-authorization-server case says "paste a token".- A registry-install 401 gets no credential hint (
Option<bool>configured-ness,call_status_kind). - The company half of the sign-in id is percent-escaped and split on the first
/, so any server name round-trips. - An unknown or consumed state renders the 400 "Sign-in expired" page; other exchange errors stay 502.
- The matched tinymcp prose is pinned by the
include_str!test, so a reword fails CI instead of silently changing a status. Typed variants remain the right tinymcp-side follow-up. - One shared
(name, configured)+ secrets snapshot per company observer;grantedcomputed once as borrowed refs; the registry-emptiness check kept for the unbuildable-set reason, which holds. skill_set::resolve_companyis test-only; the threesrc/mcp/subdirectories have READMEs.
CI is green on every lane at this head. One non-blocking nit inline on observe.rs.
| /// What classifying and scrubbing a failure needs of the declared servers: who | ||
| /// is configured, and every credential value to scrub. Held once per company | ||
| /// observer, not per agent. | ||
| #[derive(Debug, PartialEq, Eq)] |
There was a problem hiding this comment.
Nit (non-blocking): Debug over every credential value. ObservedServers.secrets is every declared server's bearer, header, query-param and OAuth token value, and this derive prints them all in the clear on {:?}. Nothing formats it today (AgentMcpObserver and McpCallObserver derive no Debug), so it is a footgun rather than a leak: the first #[derive(Debug)] added to AgentMcpObserver, or a {:?} in a tracing line or an assert_eq! on the snapshot, dumps the company's MCP credentials into logs or test output.
share_servers only needs PartialEq. Drop Debug, or give it a redacting impl the way tinymcp's OAuthBundle and this crate's CompanyAgent do.
2 participants
Summary
Moves OpenCompany's skills and MCP machinery onto the shared
tinyskills(v0.2.8) andtinymcp(v0.4.0) libraries and deletes the host copies. This is step 1 toward many isolated agents on one shared OpenHuman runtime.Closes #2576
Skills
tinyskills; OpenCompany's copies are gone.load_skill_setservice.resolvetakes the baseline and library as explicit inputs.SkillLibrarysource seam resolves the shared skill library per host. The library ships as packaged files beside the desktop app and in containers, which fixes the empty shared registry on desktop and provisioned tenants.MCP
crate::redact.crate::mcp, with one per-agent seam (resolve_for_agent -> AgentMcp) that feeds each agent'sAgentSpec.OcMcpCallToolwrapper is removed; the MCP tool scope it fed is kept and pinned by tests.OAuthFlowand registers its client as "OpenCompany".mcp.jsonis read through tinymcp's config document.Pins
vendor/openhuman→9aebda6e57(tinyhumansai/openhuman main, with tinymcp v0.4.0 and tinyskills v0.2.8).vendor/tinyhivemind→83080c18b0(build(deps): pin openhuman at 9aebda6e57 with tinymcp v0.4.0 and tinyskills v0.2.8 tinyhivemind#111, pinned to the same OpenHuman).API Or Behavior Changes
client_name = "OpenCompany", as before the tinymcp swap (tinymcp's default is "TinyMCP").extra_frontmatterlines (for examplelicense:,allowed-tools:) are now kept; before they were dropped.main(tests/snapshots/skill-pins.txt, unchanged).main.Tests
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warnings(with--locked), plus-p opencompany-core --no-deps --features openhuman,mcp,composio,acp --all-targetscargo build --all-targetswas not run separately;cargo check --locked --all-features --all-targetsand the test builds below cover the same targetscargo test, scoped:cargo test --locked -p opencompany-core --lib: 5968 passed, 1 ignoredcargo test --locked -p opencompany-core --features openhuman --testscargo test --locked --manifest-path crates/opencompany-app/Cargo.tomlbash scripts/ci/assert-no-duplicated-openhuman.sh,assert-feature-lanes,assert-desktop-features,assert-integration-targets-run.shclient_name = "OpenCompany";Manual verification (staging, serve host + console):
mcp.jsonerrors in the console editor, the config API and at boot;Documentation
crate::mcpand the skills sources are updated in place:src/mcpmap, call outcomes, console OAuth, the bundle reader, the library sources and the shared loader, and the tinyskills calls behind parse, scan, upload and materialize.