diff --git a/docs/modules/server/connections.md b/docs/modules/server/connections.md index 2da8173ae..79d601637 100644 --- a/docs/modules/server/connections.md +++ b/docs/modules/server/connections.md @@ -109,6 +109,49 @@ Two consequences worth knowing: probe is a network call with no injection point, and the removed gate lived on the far side of it, which is how it survived unasserted. +## The capabilities panel's Composio verdict is a tier, not a stored token (issue #886) + +`GET …/capabilities` is the panel an operator checks first when a tool looks +missing, so a wrong answer there sends the whole debugging session the wrong +way. It used to compute its Composio verdict from `composio::token_configured`, +which reads exactly one secret slot — the BYO override `composio/token`. + +The credential is resolved over **three** tiers, and the toolbelt gates on all +three (`composio::resolve_credential`, the seam issue #586 established): the BYO +override, then the company's own TinyHumans key, then this instance's platform +identity. On a hosted tenant nobody pastes a BYO token — the third tier answers, +and the tools wire up and are ready to attempt calls. Credential resolution +proves bearer presence, not that a later call succeeds — that is the probe's +job (above), a separate axis. The one-tier probe reported `false` throughout. + +So the route now sends **both**, answering two different questions: + +| Field | Question | Shape | +| --- | --- | --- | +| `composioTokenConfigured` | did *this company* paste a BYO token? | boolean, unchanged meaning | +| `composioCredentialSource` | which Composio credential tier resolves? | `attested` for the projected platform identity, `company` for the company's TinyHumans key, `static` for a BYO or static instance key, and `none` when no credential resolves | + +Three properties are load-bearing: + +- **The tier comes from the resolver, never from a second copy of its + precedence.** This is the same rule the `GET …/composio` status route already + follows; #886 is the copy that route's migration missed. The console must + never be able to name a tier the agents are not on. +- **An unreadable secret store omits the field**, rather than reporting `none`. + `none` is a verdict — "nothing resolves, no tools are wired" — and claiming it + on a transient hiccup sends an operator to paste a token they already have. + The console treats an absent field as unknown and must not render it in the + alarm colour. +- **It is a resolution verdict, not a liveness one.** `attested` says a bearer + can be obtained, not that Composio answered or that any account is connected. + `GET …/connections` above is the axis that answers those, and a company with a + valid bearer and zero connections is a working empty account, not a fault. + +The evidence pack the planning station builds reads the same resolver, for the +same reason: it used to tell operators "this company has no Composio credential, +so no Composio account can be reached" on a card whose own evidence listed the +connectors as connected. + ## Releasing a connection: two routes, not interchangeable (issue #404) There are two disconnects and they act on different things: diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 228e00a36..0d7827fc8 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -969,8 +969,35 @@ export interface CapabilityStatusDto { composioGranted?: boolean; /** Whether the `composio` feature is compiled into this build at all. */ composioInBuild?: boolean; - /** Whether a per-tenant Composio token is stored — never the token itself. */ + /** + * Whether a per-tenant Composio **BYO override** token is stored under + * `composio/token` — never the token itself. + * + * Narrow on purpose, and **not** "can this company reach Composio" (issue + * #886). The BYO slot is the first of three credential tiers; on a hosted + * tenant nobody pastes one and the instance's platform identity answers, so + * this reads `false` for companies whose Composio tools are wired and + * working. Read `composioCredentialSource` for the resolution verdict. + */ composioTokenConfigured?: boolean; + /** + * Which tier this company's Composio credential actually resolves from + * (issue #886) — the same three-tier resolution the toolbelt gates on, and + * the same `credentialSource` the Composio status route reports: + * + * * `attested` — the instance's platform identity (nothing stored here); + * * `company` — the company's own TinyHumans key; + * * `static` — a pasted BYO token, or a static instance key; + * * `none` — nothing resolves, so no tools are wired. + * + * A **resolution** verdict, not a liveness one: `attested` says a bearer can + * be obtained, not that Composio answered or that any account is connected. + * + * `undefined` is **unknown** — either an older host that does not send the + * field, or one whose secret store could not be read this request. It must + * never be rendered as `none`: that is the #886 lie in the other direction. + */ + composioCredentialSource?: "attested" | "company" | "static" | "none"; /** * Metered web search (issue #238): whether the company **explicitly** grants * the `search` namespace (a `*` wildcard does not count). Every call is a diff --git a/frontend/src/views/UsageView.tsx b/frontend/src/views/UsageView.tsx index c4b726479..620fc0781 100644 --- a/frontend/src/views/UsageView.tsx +++ b/frontend/src/views/UsageView.tsx @@ -304,7 +304,7 @@ const NAMESPACE_LABELS: Record = { }; // Badge variant subset the media status row uses. -type BadgeVariant = "default" | "secondary" | "destructive" | "outline"; +export type BadgeVariant = "default" | "secondary" | "destructive" | "outline"; /** * The media-generation capability (issue #109) is opt-in per tool grant and @@ -341,9 +341,8 @@ function mediaStatus(caps: CapabilityStatusDto): { label: string; variant: Badge /** * The Composio capability (issue #110) is opt-in per tool grant and gated on a - * per-tenant OAuth token, so it gets its own status row like media. Four states: - * not compiled into this build, not granted, granted-but-awaiting-token, and - * active. Set the token from Connections. + * resolved credential, so it gets its own status row like media. Five states — + * see {@link composioStatus}. */ function ComposioStatusRow({ caps }: { caps: CapabilityStatusDto }) { const { label, variant } = composioStatus(caps); @@ -352,8 +351,10 @@ function ComposioStatusRow({ caps }: { caps: CapabilityStatusDto }) {
Composio integrations

- Gmail, Slack & GitHub via Composio — opt-in, runs on the company's own OAuth - token, and every send/authorize is approved before it runs. Set the token in Connections. + Gmail, Slack & GitHub via Composio — opt-in, and every send/authorize is approved + before it runs. Runs on this company's own Composio token when one is set in + Connections; otherwise on the company's TinyHumans key, or on the platform identity + this instance already carries.

@@ -363,11 +364,31 @@ function ComposioStatusRow({ caps }: { caps: CapabilityStatusDto }) { ); } -function composioStatus(caps: CapabilityStatusDto): { label: string; variant: BadgeVariant } { +/** + * The Composio row's five states, in order (issue #886). + * + * The credential is resolved over three tiers — a BYO Composio token, the + * company's TinyHumans key, this instance's platform identity — so "is a token + * stored" is the wrong question to render. This reads `composioCredentialSource`, + * the tier the host says the toolbelt actually resolves. + * + * The `undefined` rung is load-bearing and must stay above the `"none"` rung. + * `undefined` means the host did not answer — an older build that does not send + * the field, or one whose secret store could not be read — and falling through + * it into the destructive branch is exactly the bug #886 was filed about: a red + * "no credential" badge over a Composio account that is working. Unknown is + * shown as unknown, and never in the alarm colour. + */ +export function composioStatus(caps: CapabilityStatusDto): { + label: string; + variant: BadgeVariant; +} { if (caps.composioInBuild === false) return { label: "Not in this build", variant: "outline" }; if (!caps.composioGranted) return { label: "Not granted", variant: "secondary" }; - if (!caps.composioTokenConfigured) - return { label: "Awaiting token", variant: "destructive" }; + if (caps.composioCredentialSource === undefined) + return { label: "Couldn't check", variant: "outline" }; + if (caps.composioCredentialSource === "none") + return { label: "Awaiting credential", variant: "destructive" }; return { label: "Active", variant: "default" }; } diff --git a/frontend/test/unit/composio-capability-status.test.ts b/frontend/test/unit/composio-capability-status.test.ts new file mode 100644 index 000000000..c10f76530 --- /dev/null +++ b/frontend/test/unit/composio-capability-status.test.ts @@ -0,0 +1,91 @@ +/** + * Issue #886 — the Composio row on the Usage view must never paint a working + * connector red. + * + * The credential resolves over three tiers (a BYO Composio token, the company's + * TinyHumans key, this instance's platform identity). The row used to read + * `composioTokenConfigured`, which answers only the first, so a hosted tenant + * running on the platform identity got "Awaiting token" in the alarm colour + * while its agents were calling `GITHUB_*` tools successfully. + * + * The state that matters most here is the one with no obvious label: the host + * not answering. It is a separate rung above `"none"` precisely so it cannot + * fall through into the destructive branch and re-create the bug. + */ +import { describe, expect, it } from "vitest"; + +import type { CapabilityStatusDto } from "@/api/types"; +import { composioStatus } from "@/views/UsageView"; + +/** A granted, in-build company — the only shape the credential rungs are reached from. */ +function granted(over: Partial = {}): CapabilityStatusDto { + return { + configured: false, + composioInBuild: true, + composioGranted: true, + ...over, + }; +} + +describe("composioStatus", () => { + it("reports a build without the feature before anything else", () => { + expect( + composioStatus(granted({ composioInBuild: false, composioCredentialSource: "attested" })), + ).toEqual({ label: "Not in this build", variant: "outline" }); + }); + + it("reports an ungranted company without consulting the credential", () => { + expect( + composioStatus(granted({ composioGranted: false, composioCredentialSource: "attested" })), + ).toEqual({ label: "Not granted", variant: "secondary" }); + }); + + /** + * The #886 regression guard. An unanswered host is unknown, not broken — + * and specifically not `destructive`, which is the colour that sent the + * original debugging in the wrong direction. + */ + it("reports an unanswered host as unknown, never as an alarm", () => { + const status = composioStatus(granted({ composioCredentialSource: undefined })); + expect(status.label).toBe("Couldn't check"); + expect(status.variant).not.toBe("destructive"); + }); + + it("reports a genuinely unresolvable credential as the destructive state", () => { + expect(composioStatus(granted({ composioCredentialSource: "none" }))).toEqual({ + label: "Awaiting credential", + variant: "destructive", + }); + }); + + /** + * All three resolving tiers are Active. `attested` is the hosted shape the + * issue was reported against, and it is the one the old code got wrong: + * nothing is stored on the instance, so `composioTokenConfigured` is `false` + * while the toolbelt is fully wired. + */ + it.each(["attested", "company", "static"] as const)( + "reports a resolved `%s` credential as active", + (source) => { + expect( + composioStatus(granted({ composioCredentialSource: source, composioTokenConfigured: false })), + ).toEqual({ label: "Active", variant: "default" }); + }, + ); + + /** + * The narrow legacy field must not be able to steer the verdict in either + * direction: it answers "did somebody paste a BYO token", which is a + * different question from "does a credential resolve". + */ + it("ignores the BYO-token flag once the resolver has answered", () => { + expect( + composioStatus(granted({ composioTokenConfigured: true, composioCredentialSource: "none" })) + .variant, + ).toBe("destructive"); + expect( + composioStatus(granted({ composioTokenConfigured: false, composioCredentialSource: "attested" })) + .label, + ).toBe("Active"); + }); +}); diff --git a/src/company/composio.rs b/src/company/composio.rs index 415614cc9..b9db94cbd 100644 --- a/src/company/composio.rs +++ b/src/company/composio.rs @@ -114,7 +114,29 @@ pub async fn resolve_credential( }) } -/// Whether a non-empty per-tenant token is stored — never the token itself. +/// Whether a non-empty **BYO override** token is stored under [`TOKEN_KEY`] — +/// never the token itself. +/// +/// ## This is not "can this company reach Composio" (issue #886) +/// +/// It answers exactly one question about exactly one secret slot: did somebody +/// paste a token into the company's own [`TOKEN_KEY`]. That is the *first* tier +/// of three. [`resolve_credential`] falls through it to the company's own +/// TinyHumans key and then to this instance's platform identity, and on a hosted +/// tenant it is the third tier that answers — nobody pastes a BYO token there. +/// So `false` from here is routinely true of a company whose Composio tools are +/// wired and working, which is precisely what #886 was filed about: the +/// capabilities panel reported `composioTokenConfigured: false` while agents +/// were calling `GITHUB_*` tools successfully in the same session. +/// +/// **If you want to know whether Composio will work, call +/// [`resolve_credential`] and ask the returned [`Credential`] — `configured()` +/// for the boolean, [`source`](Credential::source) for the tier.** That is the +/// same derivation the toolbelt gates on +/// ([`TenantComposio::resolve`](crate::harness::composio::TenantComposio::resolve)), +/// so it cannot disagree with what the agents actually hold. Use this function +/// only where the BYO slot itself is the subject — a console field that says +/// whether *this company pasted a token*, not whether it has one. pub async fn token_configured(company: &CompanyId, secrets: &dyn SecretStore) -> Result { Ok(secrets .get(company, TOKEN_KEY) diff --git a/src/harness/build.rs b/src/harness/build.rs index 1b3a9ec5e..cc95df2a2 100644 --- a/src/harness/build.rs +++ b/src/harness/build.rs @@ -398,12 +398,16 @@ pub fn build_agent( // `media`, the catch-all `*` does NOT grant it, so a broadly-permissioned // company never accidentally hands its agents a live account-reaching // surface; it must opt in by name. - // 2. a resolved per-tenant token on the deps (`deps.composio`), read from the - // company secret store by `HarnessPool::ensure` — never an env/platform - // key. The backend derives the Composio entity from THIS token, so it is - // the entire tenant-isolation lever. + // 2. a resolved credential on the deps (`deps.composio`), produced by + // `HarnessPool::ensure` through `composio::resolve_credential` — the BYO + // `composio/token` override, else the company's own TinyHumans key, else + // this instance's platform identity (issue #586). The backend derives the + // Composio entity from whichever tier answered, so this resolution is the + // entire tenant-isolation lever. It is NOT "a stored token": on a hosted + // tenant nobody pastes one and the platform identity is what wires the + // tools (issue #886). // - // Granted-but-tokenless wires nothing and warns (fail-closed). The + // Granted-but-credential-less wires nothing and warns (fail-closed). The // `authorize` / `execute` tools additionally park for operator approval via // the `ApprovalPolicy`. Gated on the `composio` feature; the default/ // `openhuman` build never compiles this. @@ -425,7 +429,12 @@ pub fn build_agent( None => tracing::warn!( company = %company, agent = %manifest_agent.id, - "[build] agent explicitly grants `composio` but no per-tenant Composio token is configured; composio tools NOT wired (fail-closed)" + // Issue #886: the gate is `deps.composio.is_none()`, which is a + // *resolver* outcome over three tiers (BYO `composio/token`, + // the company's TinyHumans key, this instance's platform + // identity) — not "no token is stored". Naming the stored token + // sent operators to paste one they did not need. + "[build] agent explicitly grants `composio` but no Composio credential could be resolved for this company; composio tools NOT wired (fail-closed)" ), } } diff --git a/src/harness/planning.rs b/src/harness/planning.rs index f5db1be07..635bbe94f 100644 --- a/src/harness/planning.rs +++ b/src/harness/planning.rs @@ -38,9 +38,15 @@ //! //! Only **names and booleans** ever enter the prompt. No credential value is //! read, let alone rendered: presence is checked with the same -//! `get(...).is_some()`-shaped probes -//! [`token_configured`](crate::company::composio::token_configured) and -//! [`auth_configured`](crate::company::mcp::auth_configured) use. +//! `get(...).is_some()`-shaped probes the read planes use — +//! [`auth_configured`](crate::company::mcp::auth_configured) for MCP, and for +//! Composio the resolver +//! [`resolve_credential`](crate::company::composio::resolve_credential) +//! reduced to its `configured()` boolean. The resolver rather than +//! [`token_configured`](crate::company::composio::token_configured) +//! deliberately: the latter reads only the BYO override slot, so on a hosted +//! tenant it is `false` for every company and the verdicts contradicted the +//! working connectors the evidence pack lists two lines above (issue #886). //! //! # No run, and therefore no lock //! @@ -625,8 +631,20 @@ struct Evidence { always_approve: Vec, /// Whether outbound email is wired at all (presence, never credentials). mail_configured: bool, - /// Whether a Composio token exists for this company (presence only). - composio_token: bool, + /// Whether **any** Composio credential resolves for this company (presence + /// only, never the value). + /// + /// Issue #886: this is the resolver's answer + /// ([`resolve_credential`](crate::company::composio::resolve_credential) + /// `.configured()`), covering all three tiers — the BYO `composio/token` + /// override, the company's own TinyHumans key, and this instance's platform + /// identity. It used to read only the first slot, so on a hosted tenant it + /// was `false` for every company, and `verify_composio` told operators "no + /// Composio account can be reached" about connectors that were working. + /// + /// Distinct from [`Self::composio_reachable`], which is "did the probe + /// answer" — a liveness fact. This one is "do we hold a bearer at all". + composio_credential: bool, } impl Evidence { @@ -769,10 +787,13 @@ async fn gather_evidence( Err(_) => Vec::new(), }; - let composio_token = - crate::company::composio::token_configured(runtime.id(), runtime.secrets().as_ref()) - .await - .unwrap_or(false); + let composio_credential = composio_credential_configured( + runtime.id(), + runtime.secrets().as_ref(), + crate::company::TinyhumansTokenSource::from_env(&crate::app::config::ProcessEnv) + .map(std::sync::Arc::new), + ) + .await; Ok(Evidence { company_name: record.manifest.company.name.clone(), @@ -791,10 +812,53 @@ async fn gather_evidence( workspace, skills, mail_configured: runtime.mail().is_some(), - composio_token, + composio_credential, }) } +/// Whether **any** Composio credential resolves for this company — presence +/// only, never the value (issue #886). +/// +/// Asks +/// [`resolve_credential`](crate::company::composio::resolve_credential), which +/// walks all three tiers: the BYO `composio/token` override, the company's own +/// TinyHumans key, and this instance's platform identity. The previous probe +/// was [`token_configured`](crate::company::composio::token_configured), which +/// reads only the first — so on a hosted tenant, where nobody pastes a BYO +/// token and the pod's identity is what wires the toolbelt, it answered `false` +/// for every company. [`verify_composio`] and [`verify_credential`] then told +/// operators "this company has no Composio credential, so no Composio account +/// can be reached" about connectors that were demonstrably working, on the same +/// evidence pack that listed those connectors as connected two lines above. +/// +/// Takes the instance identity **already resolved** rather than an +/// `&dyn EnvSource`, matching the console read planes: it keeps the tier matrix +/// testable without mutating the process environment, and avoids holding a +/// non-`Send` trait object across the await. +/// +/// A store read error is `false` — fail closed. The verdicts this feeds already +/// distinguish "no credential" from "not connected", and neither is worth +/// aborting a planning pass over; the pass degrades exactly as it does for every +/// other inventory it could not read. +async fn composio_credential_configured( + company: &crate::ports::types::CompanyId, + secrets: &dyn crate::ports::SecretStore, + token_source: Option>, +) -> bool { + match crate::company::composio::resolve_credential(company, secrets, token_source).await { + Ok(credential) => credential.configured(), + Err(err) => { + tracing::warn!( + company = %company, + error = %err, + "[planning] could not resolve the Composio credential; treating this company as \ + having none for this pass" + ); + false + } + } +} + /// Renders a bounded list of logical workspace paths from a flat node list. /// /// A local walk rather than a call into @@ -1111,7 +1175,7 @@ fn evidence_prompt(e: &Evidence) -> String { } out.push_str(&format!( "- Composio credential configured: {}\n- Composio reachable this pass: {}\n", - e.composio_token, e.composio_reachable + e.composio_credential, e.composio_reachable )); out.push_str(&format!( "- Outbound email configured: {}\n", @@ -1312,7 +1376,7 @@ fn verify_composio(e: &Evidence, name: &str) -> (PrereqStatus, String) { PrereqStatus::Satisfied, format!("{name} is connected through Composio"), ), - _ if !e.composio_token => ( + _ if !e.composio_credential => ( PrereqStatus::Missing, "this company has no Composio credential, so no Composio account can be reached — \ set one from the Connections tab" @@ -1380,7 +1444,7 @@ async fn verify_credential( }; } if key.starts_with("composio") { - return if e.composio_token { + return if e.composio_credential { ( PrereqStatus::Satisfied, "a Composio credential is configured".to_string(), diff --git a/src/harness/planning/test.rs b/src/harness/planning/test.rs index 104c8738a..7e4338866 100644 --- a/src/harness/planning/test.rs +++ b/src/harness/planning/test.rs @@ -24,6 +24,7 @@ use tinyagents::{Result as TaResult, TinyAgentsError}; use super::*; use crate::company::CompanyManifest; use crate::ports::types::CompanyId; +use tempfile; // --------------------------------------------------------------------------- // A scripted model @@ -220,7 +221,7 @@ fn evidence() -> Evidence { ], skills: vec!["writing".to_string()], mail_configured: false, - composio_token: true, + composio_credential: true, } } @@ -504,7 +505,7 @@ fn composio_distinguishes_no_credential_from_no_account() { assert_eq!(verify_composio(&e, "github").0, PrereqStatus::Missing); let mut e = evidence(); - e.composio_token = false; + e.composio_credential = false; let (status, note) = verify_composio(&e, "gmail"); assert_eq!(status, PrereqStatus::Missing); assert!( @@ -513,6 +514,197 @@ fn composio_distinguishes_no_credential_from_no_account() { ); } +// --------------------------------------------------------------------------- +// Issue #886: the evidence pack's Composio credential is the resolver's answer +// --------------------------------------------------------------------------- + +/// An in-memory secret store, mirroring the fixtures in `company::composio` and +/// `company::company_key`. +#[derive(Default)] +struct MemSecrets { + map: std::sync::Mutex>, +} + +#[async_trait] +impl crate::ports::SecretStore for MemSecrets { + async fn get( + &self, + _c: &CompanyId, + key: &str, + ) -> crate::Result> { + Ok(self + .map + .lock() + .unwrap() + .get(key) + .map(|v| crate::ports::types::SecretValue(v.clone()))) + } + async fn set( + &self, + _c: &CompanyId, + key: &str, + value: crate::ports::types::SecretValue, + ) -> crate::Result<()> { + self.map.lock().unwrap().insert(key.to_string(), value.0); + Ok(()) + } +} + +/// A store whose reads always fail. +struct BrokenSecrets; + +#[async_trait] +impl crate::ports::SecretStore for BrokenSecrets { + async fn get( + &self, + _c: &CompanyId, + _key: &str, + ) -> crate::Result> { + Err(crate::error::OpenCompanyError::Store("boom".into())) + } + async fn set( + &self, + _c: &CompanyId, + _key: &str, + _value: crate::ports::types::SecretValue, + ) -> crate::Result<()> { + Err(crate::error::OpenCompanyError::Store("boom".into())) + } +} + +/// The instance identity a hosted pod carries. Built directly, so the matrix +/// never touches the process environment. +fn platform_identity( + path: impl Into, +) -> Arc { + Arc::new(crate::company::TinyhumansTokenSource::projected_file(path)) +} + +/// The hosted shape, which is the whole of issue #886: **no** BYO +/// `composio/token` is stored, and the pod's platform identity is what the +/// toolbelt resolves. The evidence pack must say a credential exists. +/// +/// The old probe read only the BYO slot, so it answered `false` here — and the +/// verdicts below then announced "no Composio account can be reached" about a +/// company whose GitHub connector was working in the same session. +#[tokio::test] +async fn a_hosted_tenant_with_no_pasted_token_still_has_a_composio_credential() { + let company = CompanyId::new("acme"); + let secrets = MemSecrets::default(); + + // Create a temp file with a test token so the projected_file source has + // a path that exists, matching the pattern used in server/ops/composio.rs. + let token_dir = tempfile::Builder::new() + .prefix("oc-harness-test-") + .tempdir() + .expect("tempdir"); + let token_path = token_dir.path().join("token"); + std::fs::write(&token_path, "test-tinyhumans-token").expect("write token"); + + // The one-tier probe the field used to be. Kept in the assertion because it + // is the contradiction the issue reported, not merely a historical note. + assert!( + !crate::company::composio::token_configured(&company, &secrets) + .await + .unwrap(), + "nobody pastes a BYO token on a hosted tenant" + ); + assert!( + composio_credential_configured(&company, &secrets, Some(platform_identity(&token_path))) + .await, + "the platform identity is a Composio credential — it is what wires the tools" + ); +} + +/// The rest of the tier matrix, including the genuinely-credential-less case +/// the `missing` verdict is *supposed* to be reserved for. +#[tokio::test] +async fn the_credential_probe_walks_every_tier() { + let company = CompanyId::new("acme"); + let secrets = MemSecrets::default(); + + // Create a temp file with a test token so the projected_file source has + // a path that exists, matching the pattern used in server/ops/composio.rs. + let token_dir = tempfile::Builder::new() + .prefix("oc-harness-test-") + .tempdir() + .expect("tempdir"); + let token_path = token_dir.path().join("token"); + std::fs::write(&token_path, "test-tinyhumans-token").expect("write token"); + + // Nothing stored, no instance identity — the only shape that is really + // credential-less. + assert!(!composio_credential_configured(&company, &secrets, None).await); + + // The company's own TinyHumans key answers with no instance identity at all. + crate::company::company_key::store_key(&company, &secrets, "th_company") + .await + .unwrap(); + assert!(composio_credential_configured(&company, &secrets, None).await); + + // A pasted BYO token also answers on its own. + let byo = MemSecrets::default(); + crate::company::composio::store_token(&company, &byo, "cmp_byo") + .await + .unwrap(); + assert!(composio_credential_configured(&company, &byo, None).await); + + // An unreadable store fails closed rather than aborting the pass. + assert!( + !composio_credential_configured( + &company, + &BrokenSecrets, + Some(platform_identity(&token_path)) + ) + .await + ); +} + +/// The operator-facing sentence, end to end on the hosted shape. +/// +/// `verify_composio`'s no-credential arm was written for the right concept and +/// only ever got the wrong boolean — but the sentence it emits is the actual +/// harm the issue reports ("no Composio account can be reached" printed onto a +/// card for a company whose GitHub connector worked), so it is pinned against +/// the real function rather than a restatement of it. +#[tokio::test] +async fn a_hosted_tenant_stops_being_told_no_composio_account_can_be_reached() { + let company = CompanyId::new("acme"); + let secrets = MemSecrets::default(); + + // Create a temp file with a test token so the projected_file source has + // a path that exists, matching the pattern used in server/ops/composio.rs. + let token_dir = tempfile::Builder::new() + .prefix("oc-harness-test-") + .tempdir() + .expect("tempdir"); + let token_path = token_dir.path().join("token"); + std::fs::write(&token_path, "test-tinyhumans-token").expect("write token"); + + let mut e = evidence(); + e.composio_credential = + composio_credential_configured(&company, &secrets, Some(platform_identity(&token_path))) + .await; + + // A provider that IS connected through Composio is satisfied. + assert_eq!(verify_composio(&e, "notion").0, PrereqStatus::Satisfied); + + // One that is not still reports the honest gap — the missing *account*, + // not a missing credential. That distinction is the whole value of the + // verdict: one sends the operator to connect a provider, the other to paste + // a token they do not need. + let (status, note) = verify_composio(&e, "gmail"); + assert_eq!(status, PrereqStatus::Missing); + assert!( + note.contains("no Composio account is connected"), + "the gap is the account, not the credential: {note}" + ); + assert!( + !note.contains("no Composio credential"), + "a hosted tenant has a credential; saying otherwise is issue #886: {note}" + ); +} + /// Both halves of the MCP union, and the disabled case — which is its own /// verdict because the fix is one toggle rather than adding a server. #[test] diff --git a/src/server/ops/capabilities.rs b/src/server/ops/capabilities.rs index 567ccac3f..046530128 100644 --- a/src/server/ops/capabilities.rs +++ b/src/server/ops/capabilities.rs @@ -14,6 +14,7 @@ use axum::routing::get; use serde::Serialize; use crate::AppState; +use crate::company::credentials::{CredentialSource, TinyhumansTokenSource}; use crate::company::runtime::CompanyRuntime; use crate::metering::capability::{CapabilityPlan, tokens_in}; use crate::ports::now_millis; @@ -75,9 +76,40 @@ struct CapabilityStatusDto { composio_granted: bool, /// Whether the `composio` feature is compiled into this build at all. composio_in_build: bool, - /// Whether a non-empty per-tenant Composio token is stored — never the token - /// itself. Unlike media's env credential, this is a tenant secret. + /// Whether a non-empty per-tenant Composio **BYO override** token is stored + /// under `composio/token` — never the token itself. Unlike media's env + /// credential, this is a tenant secret. + /// + /// Deliberately narrow, and **not** the answer to "can this company reach + /// Composio" (issue #886): the BYO slot is the first of three tiers, and on + /// a hosted tenant the third one answers, so this reads `false` for a + /// company whose Composio tools are wired and working. Read + /// [`Self::composio_credential_source`] for the resolution verdict; this + /// field is retained with its original meaning for the console surface that + /// asks whether *this company pasted a token*. composio_token_configured: bool, + /// Which tier this company's Composio credential actually resolves from + /// (issue #886) — `attested` (the instance's platform identity), `company` + /// (the company's own TinyHumans key), `static` (a pasted BYO token or a + /// static instance key), or `none` (nothing resolves, so no tools are + /// wired). + /// + /// Sourced from + /// [`resolve_credential`](crate::company::composio::resolve_credential) — + /// the same derivation the toolbelt gates on — rather than a second copy of + /// its precedence, so the console can never name a tier the agents are not + /// on. Matches the `credentialSource` field + /// [`ops::composio`](crate::server::ops::composio) already reports. + /// + /// A **resolution** verdict, not a liveness one: `attested` says a bearer + /// can be obtained, not that Composio answered or that any account is + /// connected. `GET …/connections` is the axis that answers those. + /// + /// Omitted entirely when the secret store could not be read — an unknown + /// answer is not `none`, and reporting a confident "no credential" for a + /// transient store hiccup is the same class of lie #886 is about. + #[serde(skip_serializing_if = "Option::is_none")] + composio_credential_source: Option, /// Metered web search (issue #238): whether this company **explicitly** /// grants the `search` namespace (a `*` wildcard does NOT count). search_granted: bool, @@ -158,6 +190,13 @@ struct OptInFlags { media_granted: bool, composio_granted: bool, composio_token_configured: bool, + /// The resolved Composio credential tier (issue #886), or `None` when it + /// could not be determined. Travels on the flags rather than being computed + /// per DTO site because the DTO is built in two places, and a field wired + /// into one of them alone reports honestly for a company with no plan and + /// lies to every company that has one — the failure the issue #567 test + /// below exists to catch. + composio_credential_source: Option, search_granted: bool, search_daily_call_cap: u32, repo_granted: bool, @@ -170,6 +209,10 @@ impl OptInFlags { media_granted: false, composio_granted: false, composio_token_configured: false, + // `None` (undetermined), never `Some(CredentialSource::None)`: + // there is no company record to resolve a credential for, which is + // not the same answer as "no credential resolves". + composio_credential_source: None, search_granted: false, search_daily_call_cap: crate::company::DEFAULT_SEARCH_DAILY_CALLS, repo_granted: false, @@ -194,6 +237,7 @@ fn unconfigured(flags: OptInFlags) -> CapabilityStatusDto { composio_granted: flags.composio_granted, composio_in_build: cfg!(feature = "composio"), composio_token_configured: flags.composio_token_configured, + composio_credential_source: flags.composio_credential_source, search_granted: flags.search_granted, search_in_build: cfg!(feature = "openhuman"), search_credential_configured: search_credential_configured(), @@ -203,6 +247,57 @@ fn unconfigured(flags: OptInFlags) -> CapabilityStatusDto { } } +/// Which tier this company's Composio credential resolves from (issue #886), or +/// `None` when the secret store could not be read. +/// +/// Asks +/// [`resolve_credential`](crate::company::composio::resolve_credential) rather +/// than restating its precedence. The three-tier resolution — BYO +/// `composio/token`, then the company's own TinyHumans key, then this instance's +/// platform identity — is the *same* one +/// [`TenantComposio::resolve`](crate::harness::composio::TenantComposio::resolve) +/// gates the toolbelt on, and the whole point of #886 is that this panel had a +/// second, one-tier copy of the question that disagreed with it. There must be +/// exactly one derivation, and this is not it — it is a caller of it. +/// +/// Takes the instance identity **already resolved** rather than an `&dyn +/// EnvSource`, mirroring +/// [`ops::composio`](crate::server::ops::composio)'s `credential_source_for`: a +/// trait object with no `Send + Sync` bound held across the await below makes +/// the whole handler future non-`Send`, which axum rejects. Passing the resolved +/// value also keeps the tier matrix testable without mutating the process +/// environment. +/// +/// A store error yields `None` and a warning, never `Some(CredentialSource::None)`. +/// The rest of `/capabilities` is budget and tier data with nothing to do with +/// Composio, so failing the whole response would be the wrong trade — but +/// answering "no credential" for a transient hiccup would send an operator to +/// paste a token they already have, which is the #886 failure in the other +/// direction. An omitted field is the only honest "we do not know". +async fn composio_credential_source( + runtime: &CompanyRuntime, + token_source: Option>, +) -> Option { + match crate::company::composio::resolve_credential( + runtime.id(), + runtime.secrets().as_ref(), + token_source, + ) + .await + { + Ok(credential) => Some(credential.source()), + Err(err) => { + tracing::warn!( + company = %runtime.id(), + error = %err, + "[capabilities] could not resolve the Composio credential tier; omitting \ + `composioCredentialSource` rather than reporting a confident `none`" + ); + None + } + } +} + /// Whether a MANAGED search credential (issue #238) is resolvable from the /// environment on this build. Env-only, never a tenant secret, matching the /// harness's fail-closed resolution. Off the harness feature this is always @@ -255,6 +350,23 @@ async fn effective_status(runtime: &CompanyRuntime) -> Result Result AppState { + state_with(home, manifest_toml, None).await + } + + /// [`state_with_manifest`], optionally over a caller-supplied + /// [`SecretStore`](crate::ports::SecretStore) — the seam the issue #886 + /// store-error case needs, since an unreadable store is the one input the + /// filesystem-backed default cannot produce. + async fn state_with( + home: &std::path::Path, + manifest_toml: &str, + secrets: Option>, + ) -> AppState { use crate::ports::CompanyStore; let manifest: CompanyManifest = toml::from_str(manifest_toml).unwrap(); let store = FsCompanyStore::new(home.to_path_buf()); @@ -380,11 +506,11 @@ mod tests { }) .await .unwrap(); - let runtime = RuntimeBuilder::new(home.to_path_buf(), manifest) - .with_id(id.clone()) - .build() - .await - .unwrap(); + let mut builder = RuntimeBuilder::new(home.to_path_buf(), manifest).with_id(id.clone()); + if let Some(secrets) = secrets { + builder = builder.with_secrets(secrets); + } + let runtime = builder.build().await.unwrap(); let state = AppState::new(AppConfig::default()); state.registry().insert(id, std::sync::Arc::new(runtime)); crate::server::test_support::seed_fixed_admin(&state, "acme").await; @@ -702,4 +828,236 @@ mod tests { assert_eq!(total["remainingTokens"], 50_000); assert_eq!(total["exhausted"], false, "250k < 300k is under budget"); } + + // ---- issue #886: the Composio verdict comes from the resolver ----------- + + const GRANTS_COMPOSIO: &str = + "[company]\nname = \"Acme\"\n[policy]\nmode = \"full\"\n[tools]\nallow = [\"composio\"]\n"; + + /// A store whose reads always fail — the transient-hiccup case, mirroring + /// `company_key`'s own fixture. + struct BrokenSecrets; + + #[async_trait::async_trait] + impl crate::ports::SecretStore for BrokenSecrets { + async fn get( + &self, + _c: &CompanyId, + _key: &str, + ) -> crate::Result> { + Err(crate::error::OpenCompanyError::Store("boom".into())) + } + async fn set( + &self, + _c: &CompanyId, + _key: &str, + _value: crate::ports::types::SecretValue, + ) -> crate::Result<()> { + Err(crate::error::OpenCompanyError::Store("boom".into())) + } + } + + /// The instance identity the platform hands a hosted pod. Built directly + /// rather than through `from_env` so the tier matrix never touches the + /// process environment. + fn platform_identity() -> std::sync::Arc { + std::sync::Arc::new(TinyhumansTokenSource::projected_file( + "/var/run/secrets/tinyhumans.ai/token", + )) + } + + /// The whole of issue #886 in one test: the panel's Composio verdict must + /// walk **all three** credential tiers, not just the BYO slot. + /// + /// The hosted case is the one that was wrong. Nobody pastes a + /// `composio/token` on a hosted tenant — the pod's platform identity + /// answers, the toolbelt wires up, the agents call `GITHUB_*` — and the + /// old one-tier probe called that `false`, sending an operator looking for + /// a missing credential that was never missing. + #[tokio::test] + async fn the_composio_verdict_walks_every_credential_tier() { + use crate::company::{company_key, composio}; + + let home_dir = home(); + let home = home_dir.path().to_path_buf(); + let state = state_with_manifest(&home, GRANTS_COMPOSIO).await; + let runtime = state.registry().get(&CompanyId::new("acme")).unwrap(); + let secrets = runtime.secrets().clone(); + + // Nothing stored and no instance identity — fail closed, and say so. + assert_eq!( + super::composio_credential_source(runtime.as_ref(), None).await, + Some(CredentialSource::None), + "with no tier able to answer, `none` is the honest verdict" + ); + + // The hosted shape: nothing stored, the pod's projected identity + // answers. This is the reported bug. + assert_eq!( + super::composio_credential_source(runtime.as_ref(), Some(platform_identity())).await, + Some(CredentialSource::Attested), + ); + assert!( + !composio::token_configured(runtime.id(), secrets.as_ref()) + .await + .unwrap(), + "and the BYO slot is empty in exactly that case — the two fields \ + answer different questions, which is why the panel needs both" + ); + + // The company's own TinyHumans key outranks the instance identity. + company_key::store_key(runtime.id(), secrets.as_ref(), "th_company") + .await + .unwrap(); + assert_eq!( + super::composio_credential_source(runtime.as_ref(), Some(platform_identity())).await, + Some(CredentialSource::Company), + ); + + // A pasted BYO token outranks everything. + composio::store_token(runtime.id(), secrets.as_ref(), "cmp_byo") + .await + .unwrap(); + assert_eq!( + super::composio_credential_source(runtime.as_ref(), Some(platform_identity())).await, + Some(CredentialSource::Static), + ); + } + + /// The gate. The DTO's verdict must **equal what the resolver says**, not a + /// value this route computed for itself. + /// + /// Asserted as an equality against a live `resolve_credential` call rather + /// than against a literal, deliberately: a literal would be satisfied by a + /// second hardcoded copy of the precedence living in this file, and a second + /// copy is the entire defect. Issue #586 removed one from the sibling status + /// route; #886 is the one it missed here. + /// + /// Run across the tiers a store can produce on its own, so the equality is + /// exercised with more than one answer. + #[tokio::test] + async fn the_dto_reports_exactly_what_the_resolver_resolves() { + use crate::company::{company_key, composio}; + + let home_dir = home(); + let home = home_dir.path().to_path_buf(); + let state = state_with_manifest(&home, GRANTS_COMPOSIO).await; + let runtime = state.registry().get(&CompanyId::new("acme")).unwrap(); + let secrets = runtime.secrets().clone(); + + // The route reads the instance identity from the process environment, + // so the expectation must be derived from the same place — otherwise + // this asserts against the test host's env rather than against the + // resolver. + let resolver_says = || async { + composio::resolve_credential( + runtime.id(), + secrets.as_ref(), + TinyhumansTokenSource::from_env(&crate::app::config::ProcessEnv) + .map(std::sync::Arc::new), + ) + .await + .unwrap() + .source() + }; + + for label in ["nothing stored", "company key", "byo token"] { + match label { + "company key" => { + company_key::store_key(runtime.id(), secrets.as_ref(), "th_company") + .await + .unwrap() + } + "byo token" => composio::store_token(runtime.id(), secrets.as_ref(), "cmp_byo") + .await + .unwrap(), + _ => {} + } + let (status, dto) = get_capabilities(&state).await; + assert_eq!(status, StatusCode::OK); + assert_eq!( + dto["composioCredentialSource"], + resolver_says().await.as_str(), + "the panel must never name a tier the toolbelt is not on ({label}): {dto}" + ); + } + } + + /// Both DTO construction sites carry the field — `unconfigured()` and the + /// configured branch — per the issue #567 precedent above. A field wired + /// into one alone reports honestly for a company with no plan and lies to + /// every company that has one. + /// + /// The legacy `composioTokenConfigured` is pinned alongside it, keeping its + /// original narrow meaning: `false` with no BYO token, `true` with one. + /// Nothing about #886 changes what that field answers — only what the + /// console reads for the question it was being misused for. + #[tokio::test] + async fn both_response_paths_carry_the_credential_tier() { + use crate::company::composio; + + for manifest in [ + GRANTS_COMPOSIO, + &format!("{GRANTS_COMPOSIO}[plan]\nname = \"starter\"\n"), + ] { + let home_dir = home(); + let home = home_dir.path().to_path_buf(); + let state = state_with_manifest(&home, manifest).await; + let runtime = state.registry().get(&CompanyId::new("acme")).unwrap(); + + let (_, dto) = get_capabilities(&state).await; + assert!( + dto.get("composioCredentialSource").is_some(), + "every response states the resolved tier: {dto}" + ); + assert_eq!( + dto["composioTokenConfigured"], false, + "no BYO token pasted yet: {dto}" + ); + + composio::store_token(runtime.id(), runtime.secrets().as_ref(), "cmp_byo") + .await + .unwrap(); + let (_, dto) = get_capabilities(&state).await; + assert_eq!( + dto["composioTokenConfigured"], true, + "the legacy field keeps answering its own narrow question: {dto}" + ); + assert_eq!( + dto["composioCredentialSource"], "static", + "and a pasted token is the `static` tier: {dto}" + ); + } + } + + /// An unreadable secret store **omits** the field rather than reporting + /// `none`. + /// + /// `none` is a verdict — "no credential resolves, no tools are wired" — and + /// claiming it on a transient hiccup would send an operator to paste a token + /// they already have. That is issue #886 in the other direction, so the only + /// honest wire shape for "we do not know" is absence. The rest of the + /// response still serves: budgets and tiers have nothing to do with Composio. + #[tokio::test] + async fn an_unreadable_store_omits_the_tier_rather_than_claiming_none() { + let home_dir = home(); + let home = home_dir.path().to_path_buf(); + let state = state_with( + &home, + GRANTS_COMPOSIO, + Some(std::sync::Arc::new(BrokenSecrets)), + ) + .await; + + let (status, dto) = get_capabilities(&state).await; + assert_eq!(status, StatusCode::OK, "the response still serves: {dto}"); + assert!( + dto.get("composioCredentialSource").is_none(), + "an unknown tier is omitted, never rendered as a confident `none`: {dto}" + ); + assert_eq!( + dto["composioGranted"], true, + "the manifest-derived flags are unaffected by the store: {dto}" + ); + } }