fix(planning): a natively-stored connection no longer satisfies a prerequisite (#396) - #508
Conversation
…requisite (tinyhumansai#396) verify_connection stamped satisfied on any row with connected: true, whatever its via said. A provider connected through the Connections tab alone is stored under the host's own oauth/{provider} namespace, which nothing under src/harness/ ever reads — no agent tool resolves a credential from it. So a card whose prerequisite was "needs a connection to X" passed the check, dispatched, and failed at the tool call with no explanation, having been told the capability was there. The arm now requires composio in via, matching verify_composio. A native-only row reads missing with a note that acknowledges the stored credential rather than telling the operator to connect it again — that is not the action that helps them. unverified still outranks both and reads unknown, so an inventory outage cannot turn into a wall of missing. Tests cover native-only, composio-backed, both namespaces at once, and unverified across every via shape. Each was checked against its negative control: reverting the via guard, forcing the composio arm missing, demanding an exact [composio] match, and dropping the unverified arm each fail the matching test and no other.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughConnection prerequisite verification now requires a Composio-backed connection path. Native-only and namespace-free connections remain missing, while unverified inventory rows remain unknown. Tests cover case-insensitive lookup and mixed namespace ordering. ChangesConnection verification
Estimated code review effort: 2 (Simple) | ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Summary
Closes #396.
A card's
connectionprerequisite was satisfied by any row withconnected: true,regardless of which namespace stored it. The
oauth/{provider}catalog is thedocumented self-hosted hatch — no agent tool consumes those tokens — so a card
whose prerequisite was met that way was cleared to run against a capability that
does not exist. Its sibling
verify_composioalready requiredviato includecomposio; this bringsverify_connectionin line.Most of #396 landed in #402 (console advisory, manifest comment, hosted-tier
credentialSourcegating, state reconciliation). This is the remaining consumerthat still treated an inert credential as capability.
API Or Behavior Changes
Planning behavior only; no wire or API change.
A
connectionprerequisite backed only by a native token now reportsMissinginstead of
Satisfied. The message acknowledges the stored connection rather thanclaiming the provider is unconnected, and names the route that does work:
Composio-backed and mixed-namespace rows are unchanged (
Satisfied).unverifiedrows are unchanged (
Unknown) — an inventory outage still never reads asMissing.Self-hosted operators who natively connected a provider will see planning report the
prerequisite as missing. That is the intended correction: the credential buys no
agent capability today. If #319 wires native tokens to tools, this arm is the one to
revisit, and a comment in the code says so.
Tests
Four new unit tests in
src/harness/planning/test.rs, plus one stale fixtureassertion corrected (it asserted
Satisfiedfor a native-only provider).Every negative control was executed — the fix was broken, the suite run, the failure
observed, then restored:
viaguardMissing["composio"]unverifiedarmEach control failed exactly the test written to catch it and nothing unrelated.
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo clippy --locked --no-deps --features openhuman,tinycortex --all-targets -- -D warningscargo test --features openhuman,tinycortex --lib harness::planning::test::— 33 passed, 0 failedcargo build --all-targets: N/A — covered by the two clippy lanes above, which buildall targets in both feature configurations.
Documentation
No user-facing docs affected. The prerequisite-kind doc table in
planning.rsisupdated to record that
connectionandcomposiodiffer in wording only, not inwhat they require.
Summary by CodeRabbit