diff --git a/src/harness/planning.rs b/src/harness/planning.rs index ea73dd1c..25548b4b 100644 --- a/src/harness/planning.rs +++ b/src/harness/planning.rs @@ -1168,7 +1168,7 @@ fn evidence_prompt(e: &Evidence) -> String { /// /// | Kind | Checked against | `missing` reads as | /// | --- | --- | --- | -/// | `connection` | the `GET …/connections` projection | "GitHub is not connected — connect it from the Connections tab" | +/// | `connection` | the `GET …/connections` projection **including its `via`** | "GitHub is not connected", or "connected, but only in this host's catalog — no agent tool can use that credential" | /// | `composio` | the same projection's `via`, plus token presence | "no Composio account is connected for this" | /// | `mcp` | manifest `[[mcp_server]]` ∪ the runtime index — **both** halves | the named server is in neither | /// | `credential` | presence only: the mail handle, or a secret key that exists | "no outbound email is configured" | @@ -1182,6 +1182,16 @@ fn evidence_prompt(e: &Evidence) -> String { /// slip through during an outage and the run then fails with a real error — /// which is today's behaviour for every card, just rarer. /// +/// **`connection` and `composio` differ only in wording, not in what they +/// require.** Both are satisfied by `"composio" ∈ via` and by nothing else, +/// because Composio is the only connection path a tool actually resolves a +/// credential from. A provider connected *natively* — stored under the host's +/// own `oauth/{provider}` namespace by the Connections tab — is reported +/// `missing` with a note that says so, rather than `satisfied`: the credential +/// is real, but no agent can reach it, so a card planned against it would +/// dispatch into work it cannot do. See `verify_connection` for the arm and +/// issues #319/#396 for when that stops being true. +/// /// Permissions are checked against the **manifest** only: the tool allow-list, /// the agent's own list, and `[policy]`. Not the live grant set /// (`runtime::grants`) and not the harness [`ApprovalPolicy`] — those are the @@ -1243,10 +1253,31 @@ fn verify_connection(e: &Evidence, name: &str) -> (PrereqStatus, String) { unreachable, so this has not been verified either way" ), ), - Some((true, via, _)) => ( + Some((true, via, _)) if via.iter().any(|v| v == "composio") => ( PrereqStatus::Satisfied, format!("{name} is connected (via {})", via.join(" + ")), ), + // Connected, but only natively: the credential sits in the host's own + // `oauth/{provider}` namespace, which the Connections tab writes and + // which nothing under `src/harness/` ever reads. No agent tool + // resolves a credential from it, so the capability this prerequisite + // asks for does not exist — stamping `satisfied` here green-lights a + // plan that cannot run (issue #396). The note acknowledges the stored + // connection instead of claiming the provider is not connected, + // because "connect it again" is not the action that helps. + // + // **This is the arm to revisit** if native tokens are ever wired + // through to tools — issue #319 owns the token-custody half. At that + // point the test becomes "via ∩ {composio, native} ≠ ∅" and the + // `composio` kind is what stays Composio-only. + Some((true, _, _)) => ( + PrereqStatus::Missing, + format!( + "{name} is connected in this host's catalog, but no agent tool uses that \ + credential — agents reach {name} through Composio, so connect it there from \ + the Connections tab" + ), + ), Some((false, _, _)) => ( PrereqStatus::Missing, format!("{name} is not connected — connect it from the Connections tab"), diff --git a/src/harness/planning/test.rs b/src/harness/planning/test.rs index 0b77cd4d..95f8ee29 100644 --- a/src/harness/planning/test.rs +++ b/src/harness/planning/test.rs @@ -344,15 +344,120 @@ fn workspace_paths_render_and_terminate() { #[test] fn a_connection_is_checked_against_the_inventory() { let e = evidence(); - assert_eq!(verify_connection(&e, "github").0, PrereqStatus::Satisfied); + assert_eq!(verify_connection(&e, "notion").0, PrereqStatus::Satisfied); // Case is not a distinction an operator should have to get right. - assert_eq!(verify_connection(&e, "GitHub").0, PrereqStatus::Satisfied); + assert_eq!(verify_connection(&e, "Notion").0, PrereqStatus::Satisfied); assert_eq!(verify_connection(&e, "slack").0, PrereqStatus::Missing); let (status, note) = verify_connection(&e, "stripe"); assert_eq!(status, PrereqStatus::Missing, "undeclared reads as missing"); assert!(note.contains("Connections tab"), "{note}"); } +/// **The arm this whole check exists for.** A provider connected *natively* is +/// stored under the host's own `oauth/{provider}` namespace, which no agent +/// tool ever reads. Reporting `satisfied` would green-light a card that +/// dispatches into work it cannot do and fails with no explanation — the exact +/// silent-wrong-answer this module's tests are here to catch (issue #396). +/// +/// The note must **acknowledge** the stored connection rather than claim the +/// provider is not connected: an operator who just completed that OAuth +/// handshake, told to "connect it", will connect it again and get nowhere. +#[test] +fn a_natively_connected_provider_is_missing_not_satisfied() { + let mut e = evidence(); + e.connections.insert( + "github".to_string(), + (true, vec!["native".to_string()], false), + ); + let (status, note) = verify_connection(&e, "github"); + assert_eq!( + status, + PrereqStatus::Missing, + "a native-only credential confers no agent capability: {note}" + ); + assert!( + note.contains("Composio"), + "the note must name the path that does work: {note}" + ); + assert!( + note.contains("is connected in this host's catalog"), + "the note must acknowledge the credential the operator already stored, \ + not tell them to connect it again: {note}" + ); + + // An empty `via` is the same verdict for the same reason — nothing in it + // names a path a tool can resolve. + e.connections + .insert("github".to_string(), (true, Vec::new(), false)); + assert_eq!(verify_connection(&e, "github").0, PrereqStatus::Missing); +} + +/// The satisfying case, and the only one: a Composio-backed connection is the +/// single path a tool actually resolves a credential from. +#[test] +fn a_composio_backed_connection_is_satisfied() { + let mut e = evidence(); + e.connections.insert( + "github".to_string(), + (true, vec!["composio".to_string()], false), + ); + let (status, note) = verify_connection(&e, "github"); + assert_eq!(status, PrereqStatus::Satisfied, "{note}"); + assert!(note.contains("composio"), "{note}"); +} + +/// Both namespaces at once is still satisfied. The check is membership, not +/// equality — a provider connected natively *and* through Composio has a +/// credential a tool can reach, and the useless native copy alongside it does +/// not take that away. +#[test] +fn a_connection_via_both_namespaces_is_satisfied() { + let mut e = evidence(); + e.connections.insert( + "github".to_string(), + ( + true, + vec!["native".to_string(), "composio".to_string()], + false, + ), + ); + assert_eq!(verify_connection(&e, "github").0, PrereqStatus::Satisfied); + + // And in the other order, because a `via` list has no guaranteed ordering. + e.connections.insert( + "github".to_string(), + ( + true, + vec!["composio".to_string(), "native".to_string()], + false, + ), + ); + assert_eq!(verify_connection(&e, "github").0, PrereqStatus::Satisfied); +} + +/// `unverified` outranks the `via` distinction in both directions. A probe that +/// did not answer cannot tell us *how* a provider is connected any more than it +/// can tell us *whether* — so the verdict is `unknown`, never the new `missing`. +#[test] +fn an_unverified_row_is_unknown_whatever_its_via_says() { + let mut e = evidence(); + for via in [ + Vec::new(), + vec!["native".to_string()], + vec!["composio".to_string()], + vec!["native".to_string(), "composio".to_string()], + ] { + e.connections + .insert("github".to_string(), (true, via.clone(), true)); + let (status, note) = verify_connection(&e, "github"); + assert_eq!( + status, + PrereqStatus::Unknown, + "via {via:?} on an unverified row: {note}" + ); + } +} + /// The failure direction that matters. A provider whose inventory could not be /// reached is **unknown**, never **missing** — a Composio outage must not make /// every card in the company unplannable.