Unified late-bound tool contract gate with compaction-safe transcript presence#4861
Unified late-bound tool contract gate with compaction-safe transcript presence#4861yh928 wants to merge 14 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesContract gate
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Model
participant ContractGateMiddleware
participant ContractResolver
participant LateBoundTool
Model->>ContractGateMiddleware: issue gated tool call
ContractGateMiddleware->>ContractResolver: fetch full contract
ContractResolver-->>ContractGateMiddleware: return contract or lookup result
ContractGateMiddleware-->>Model: return contract in tool error
Model->>ContractGateMiddleware: retry call with marker present
ContractGateMiddleware->>LateBoundTool: execute tool
LateBoundTool-->>Model: return tool result
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6236939f1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/tinyagents/contract_gate.rs (1)
174-184: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid bulk allocation of tool message texts.
Collecting
m.text()for all tool messages into an intermediateVec<String>forces a peak memory allocation proportional to the total size of all tool messages in the transcript (which can reach megabytes).You can avoid this O(N) memory spike by streaming the iterator directly into
collect_present_keysusingAsRef<str>, evaluating and dropping each message's text one at a time.♻️ Proposed refactor to stream texts without an intermediate `Vec`
Update
refresh_presentto remove the.collect():- fn refresh_present(&self, messages: &[TaMessage]) { - let tool_texts: Vec<String> = messages - .iter() - .filter(|m| matches!(m, TaMessage::Tool(_))) - .map(|m| m.text()) - .collect(); - let keys = collect_present_keys(tool_texts.iter().map(String::as_str)); - if let Ok(mut set) = self.present.lock() { - *set = keys; - } - } + fn refresh_present(&self, messages: &[TaMessage]) { + let keys = collect_present_keys( + messages + .iter() + .filter(|m| matches!(m, TaMessage::Tool(_))) + .map(|m| m.text()), + ); + if let Ok(mut set) = self.present.lock() { + *set = keys; + } + }Then generalize the helper's signature further down the file so it accepts the owned
Stringiterator:-fn collect_present_keys<'a>(texts: impl Iterator<Item = &'a str>) -> HashSet<String> { +fn collect_present_keys(texts: impl Iterator<Item = impl AsRef<str>>) -> HashSet<String> { let mut set = HashSet::new(); for text in texts { - if let Some(rest) = text.strip_prefix(MARKER_OPEN) { + if let Some(rest) = text.as_ref().strip_prefix(MARKER_OPEN) { if let Some(j) = rest.find(']') { set.insert(rest[..j].to_string()); } } } set }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/tinyagents/contract_gate.rs` around lines 174 - 184, Update refresh_present to pass the filtered tool-message text iterator directly to collect_present_keys, removing the intermediate Vec<String> allocation. Generalize collect_present_keys to accept an iterator of items implementing AsRef<str>, while preserving key collection behavior and allowing each generated text value to be dropped after processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/tinyagents/contract_gate.rs`:
- Around line 478-480: Add a verbose, grep-friendly diagnostic log in the
ContractResult::Unavailable arm of the fetch_contract match before delegating to
next.run(ctx, state, call). Include enough context to identify the target and
transient contract-fetch failure, while preserving the existing execution flow.
---
Nitpick comments:
In `@src/openhuman/tinyagents/contract_gate.rs`:
- Around line 174-184: Update refresh_present to pass the filtered tool-message
text iterator directly to collect_present_keys, removing the intermediate
Vec<String> allocation. Generalize collect_present_keys to accept an iterator of
items implementing AsRef<str>, while preserving key collection behavior and
allowing each generated text value to be dropped after processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6d9c2ae-35b5-4406-854f-586e018ae5f7
📒 Files selected for processing (3)
src/openhuman/tinyagents/contract_gate.rssrc/openhuman/tinyagents/contract_gate_tests.rssrc/openhuman/tinyagents/mod.rs
6236939 to
62d3b81
Compare
62d3b81 to
fad71b4
Compare
fad71b4 to
ef89c16
Compare
YellowSnnowmann
left a comment
There was a problem hiding this comment.
Solid, well-engineered change — the design directly addresses #4853, and deriving contract-presence from a transcript marker (rather than mutable turn state) is genuinely nice: it's correct-by-construction across summarization, microcompact, hard-trim, and durable sub-agent resume, which is strictly better than the "reset seen-state on compaction" the issue asked for. The anti-spoof prefix check and the "all same-batch calls gated" reasoning are right and are tested.
I'm requesting changes on verification gaps, not on the design. Four things I'd want resolved before approving:
1. The core behavior is untested. All 22 tests are pure logic. The network resolvers and — more importantly — the real harness wiring (before_model refreshes presence, then wrap_tool gates, in the actual middleware order from assemble_turn_harness) have no integration coverage. The gated_call_delivers_the_contract_then_the_retry_passes test manually simulates the rescan; it doesn't prove the middleware ordering actually produces that sequence. The top acceptance criterion — "repro gone" — has no automated proof. Please add at least one integration test exercising deliver → retry through assemble_turn_harness.
2. The inventory-count assertions may not run. The updated 16 / 5 counts in tests.rs sit under a comment stating they're "NOT compiled by cargo check --lib … verify/adjust the exact numbers when the test suite actually compiles." Please confirm that block actually executes in CI (link a passing run), or the counts are unverified.
3. UPPER_SNAKE false-positive is a hard-failure mode. looks_like_composio_slug gates on name shape alone. Any non-Composio tool ever named in UPPER_SNAKE_CASE would resolve to Composio(name) → toolkit_from_slug returns None → NotFound → the tool never executes and the model gets a misleading "not a valid Composio action slug" message. This relies on an unenforced lowercase-tool convention. Please add a guard (e.g. check the name against the actual registered Composio action set) or at minimum a comment documenting the invariant.
4. Contracts are delivered as tool errors — confirm they don't trip an error breaker. The NotFound path's own comment references "the repeated-tool-failure breaker." Since Found deliveries are also TaToolResult::error, a turn touching several gated targets emits multiple errors before any retry lands. Please confirm the Found deliveries don't count toward that breaker (or exempt them).
None of these are design objections — CI green + CodeRabbit clean is necessary but doesn't cover any of the above. Happy to re-review once addressed.
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
Review notes on #4861 (contract gate for #4853) — non-blocking commentReviewed the diff and read Does it address the root cause?Mechanically, yes. The gate short-circuits the first call per late-bound target each turn and hands back the full contract as a tool error, so the retry runs with the real schema in context — the same lever the issue's own diagnostic ("tell it to read the contract first") confirmed works. Verified:
Gaps / risks worth weighing
Test qualityGood — 22 pure-logic tests with real assertions: target extraction for every kind (+ missing-arg pass-through), the core deliver→retry transition, same-batch gating, marker presence across resume/compaction/anti-spoof, the Composio/MCP resolvers (found + not-found + valid-list + empty-server), list capping, and contract rendering. One caveat: the async Bottom lineSolid engineering and a faithful implementation of the issue's chosen design; merge-worthy on mechanism and quality. It does not, on its own, prove the repro is eliminated — that hinges on a live Gmail run and on Composio's real schema carrying the quoting guidance. Suggest a live smoke of the Gmail search repro before/at merge, and noting the deferred |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69d383f403
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match crate::openhuman::skills::registry::get_workflow(&config.workspace_dir, id) { | ||
| Some(def) => ContractResult::Found(render_workflow_contract(id, &def)), |
There was a problem hiding this comment.
Honor workflow profile allowlists before returning contracts
When an active agent profile scopes workflows, list_workflows, describe_workflow, and RunWorkflowTool all enforce skill_allowlist, but this new lookup goes straight to the global registry. A model that guesses a disallowed workflow_id will receive that workflow's input contract before the real run_workflow tool gets a chance to reject it, leaking metadata that the profile intentionally hides; thread the active allowlist into the gate or let the underlying tool return its scoped error.
Useful? React with 👍 / 👎.
| Ok(MiddlewareToolOutcome::Result(TaToolResult::error( | ||
| call.id, call.name, body, | ||
| ))) |
There was a problem hiding this comment.
Preserve contract bodies through tool-output rewriting
When a fetched contract is large enough to hit the existing after-tool output pipeline, returning it as an ordinary composio_execute/mcp_registry_tool_call/run_workflow error lets payload summarization, TokenJuice compaction, or the 16 KiB result cap rewrite or truncate the schema. Because the gate marker is part of that same body, the next turn can either treat a partial contract as present and execute with an incomplete schema, or lose the marker and repeatedly re-deliver; contract-gate results need an exemption or a marker/body format that survives those stages.
Useful? React with 👍 / 👎.
Codex flagged (tinyhumansai#4861 review) that a delivered contract, returned as an ordinary tool error, flows through the same output pipeline as any tool result — payload summarization, TokenJuice compaction, the 16 KiB result cap. Any of those can rewrite or truncate the body, and because the `[contract-gate:]` presence marker rides in that same body, the next turn could treat a partial contract as "present" and execute with an incomplete schema (or lose the marker and re-deliver forever). Rather than exempt deliveries from that pipeline, embed a fingerprint: the marker now carries an XXH3-64 digest of the exact bytes that follow it — `[contract-gate:<digest>:<key>[,<key>...]]`. On rescan `before_model` re-hashes those bytes and credits the key(s) ONLY when the digest still matches. A summarized / capped / rewritten body fails the check, so the gate re-delivers the full contract (fail-safe) instead of trusting a mutated one. XXH3 is fast and stable across processes (a resumed sub-agent's byte-identical history still verifies); non-cryptographic — it guards accidental mutation, not a forged collision. - new `xxhash-rust` dep (xxh3 feature) - `payload_digest` / `build_marker` assemble+verify the digest; the marker-to-body concatenation is owned by `deliver_body` and `prefix_with_present_marker` so the hashed bytes always equal the post-marker bytes - `present_marker(keys) -> Option<String>` becomes `prefix_with_present_marker(keys, body) -> String`; the three discovery pre-crediting call sites updated (mcp helper now returns keys, not a marker) - `collect_present_keys` parses `<digest>:<keys>` and drops the keys unless the recomputed digest matches - tests rebuilt on the real delivery path, plus new coverage: a truncated body and a rewritten body both fail the digest gate, and a digest-less marker is ignored Addresses the "preserve contract bodies through tool-output rewriting" review point on tinyhumansai#4861. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012BLvp9tiJ2bFUrcY821FY1
…contract leak) Codex flagged (tinyhumansai#4861 review) that the gate's `fetch_workflow` resolved a workflow contract straight from the global registry, while the real `run_workflow` / `describe_workflow` tools enforce the active profile's `skill_allowlist`. A model guessing a scoped-out `workflow_id` would receive that workflow's input contract from the gate — leaking metadata the profile hides. Thread the allowlist to the gate the same identity-from-the-tool way `composio_actions` already flows: `RunWorkflowTool` self-reports its `skill_allowlist` via a new `Tool::workflow_gate_allowlist` trait method, and `assemble_turn_harness` collects it into `ContractGateMiddleware`. `decide` then passes a scoped-out workflow straight through (no contract rendered) so the real tool owns the canonical "not available to the active agent profile" rejection — the gate never reveals such a workflow exists. Checked the other two gated surfaces: Composio (`composio_integrations`) and MCP-registry (`allowed_mcp_servers`) have NO equivalent leak, because their executing tools do not enforce that per-profile scope in the first place (the gate mirrors the absent enforcement, exposing nothing the model couldn't already reach by calling the tool). Only workflows enforce an allowlist the gate bypassed. - new default-None `Tool::workflow_gate_allowlist`; `RunWorkflowTool` overrides it - gate stores `workflow_allowlist`, checks it in `decide` via `workflow_allowed` - tests: scoped-out workflow not gated; allowed workflow still gated; a None allowlist gates every workflow Addresses the "honor workflow profile allowlists before returning contracts" review point on tinyhumansai#4861. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012BLvp9tiJ2bFUrcY821FY1
Reviewer tinyhumansai#1 on tinyhumansai#4861 asked for a test through the real `assemble_turn_harness` middleware ordering (not just unit-level `decide()`), exercising the whole deliver→retry so the transcript-derived presence check is validated end to end. - cfg(test)-only contract-source seam (`test_support::InjectedContracts`): `fetch_contract` first consults an injected map keyed by `GateTarget::key()`, so an integration test resolves a deterministic contract without reaching a live Composio / workflow / MCP source. The middleware is built inside `assemble_turn_harness`, so the test never holds it — the seam is global, serial-locked, and RAII-cleared. String-valued (delivered as `ContractResult::Found`) so the private `ContractResult` keeps its visibility. The whole module is `#[cfg(test)]` and absent from production builds. - integration test in `tinyagents::tests`: a mock provider issues the same gated `composio_execute` call twice (gated first attempt, then retry) with a recording stub tool, driven through `run_turn_via_tinyagents_shared` → `assemble_turn_harness`. The single assertion `executed == 1` proves BOTH halves: the gate blocked the first attempt (else 2) AND the delivered contract round-tripped through `before_model` so the retry passed (else 0, looping). Also asserts the delivery precedes the execution in the transcript. `openhuman::tinyagents` suite green (152, incl. the `adapter_inventory` middleware-count assertions — the seam adds no middleware); fmt clean. Addresses reviewer integration-test point tinyhumansai#1 on tinyhumansai#4861. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
Composio actions, MCP-registry remote tools, and Workflows expose only a thin listing to the model — an opaque dispatcher args object, or a one-line description with a possibly-empty parameter schema — while the full contract (input schema + description) lives behind a separate list/describe meta-tool. The model therefore composes a call before the real schema is in context and guesses argument formats and slugs: e.g. a Composio Gmail search sent without the quotes Gmail's query syntax needs, or an invented GMAIL_LIST_MESSAGES in place of GMAIL_FETCH_EMAILS. Add ContractGateMiddleware (src/openhuman/tinyagents/contract_gate.rs). When a gated target's contract is NOT present in the transcript, the call is short-circuited with that target's full contract as a tool error, so the retry — now with the contract in context — executes normally. Unknown targets return a not-found message plus the valid list. Kill switch OPENHUMAN_CONTRACT_GATE=0. Presence is derived from the transcript, not tracked as turn state: each delivered contract carries a [contract-gate:<key>] marker, and before every model call the gate rescans the tool-role messages and rebuilds the in-context set. This is correct by construction across resume (a contract surviving in a durable sub-agent's initial_history is not re-delivered), summarization, microcompact (tool-body blanking), and hard-trim (a contract dropped from context is re-delivered). Only tool-role messages are scanned, so a model echoing the marker cannot spoof presence. Registered in assemble_turn_harness as a dual-trait middleware (the SchemaGuardMiddleware shape): before_model refreshes presence, an outer wrap_tool is the gate. Every agent turn — chat, channel/CLI, sub-agents, and background trigger/escalation/cron/webhook/orchestration paths — routes through run_turn_via_tinyagents_shared -> assemble_turn_harness, so each run gets its own instance and nothing leaks across turns or between concurrent agents. The MCP config/legacy bridge (mcp_call_tool) is left as follow-up: its McpServerRegistry handle is private to the tool with no ambient accessor. Issue tinyhumansai#4853 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eaker exemption Follow-up on the contract gate (ef89c16) addressing review feedback. Identify Composio per-action tools authoritatively, not by name shape. Tool::composio_action_slug() (default None; overridden only by ComposioActionTool) reports the slug; assemble_turn_harness collects the registered set and the gate treats a bare-name call as a Composio action only when its name is a real registered slug. A non-Composio UPPER_SNAKE tool can no longer be mistaken for one and gated into never executing. Removes the looks_like_composio_slug name-shape heuristic. Pre-credit full-schema discovery tools so an agent that already read the real contract pays no redundant re-delivery. describe_workflow, mcp_registry_list_tools, and composio_list_tools (only when the model reads the full JSON, not the thin markdown rendering) lead their output with one [contract-gate:...] marker packing every target they fully described — comma-joined keys, still a single marker at the START so the fixed-length prefix scan is unchanged. A thin listing never emits a marker (crediting it would mark a not-yet-seen contract present and defeat the gate). collect_present_keys now splits a multi-key marker; present_marker + composio_key/mcp_key/workflow_key build them from the same GateTarget keys the gate checks presence against. Exempt Found contract deliveries from the repeated-tool-failure breaker. A delivery is a tool error but not a failure — it hands over the contract and expects a retry — and is bounded to at most once per tool while the contract stays in context. is_contract_delivery() gates the exemption on the leading marker, so a NotFound "no such target" message (no marker) still trips the breaker when a model loops on a bogus slug. Correct the stale adapter-inventory comment: those counts are asserted by a live test that runs in CI's Rust Core Coverage job, not a deferred #[cfg(test)] block. Issue tinyhumansai#4853 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012BLvp9tiJ2bFUrcY821FY1
An end-to-end run against a self-hosted instance showed a weak sub-agent model read a delivered contract (the tool's input SCHEMA) as if it were the tool's OUTPUT, concluded "no results", and stopped without retrying — so the gated tool never actually ran. Prefix every Found delivery, right after the `[contract-gate:<key>]` marker, with a banner stating plainly the tool did NOT execute and the same call must be re-issued. The marker still leads the message, so before_model's fixed-length prefix scan is unaffected. Issue tinyhumansai#4853 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012BLvp9tiJ2bFUrcY821FY1
Codex flagged (tinyhumansai#4861 review) that a delivered contract, returned as an ordinary tool error, flows through the same output pipeline as any tool result — payload summarization, TokenJuice compaction, the 16 KiB result cap. Any of those can rewrite or truncate the body, and because the `[contract-gate:]` presence marker rides in that same body, the next turn could treat a partial contract as "present" and execute with an incomplete schema (or lose the marker and re-deliver forever). Rather than exempt deliveries from that pipeline, embed a fingerprint: the marker now carries an XXH3-64 digest of the exact bytes that follow it — `[contract-gate:<digest>:<key>[,<key>...]]`. On rescan `before_model` re-hashes those bytes and credits the key(s) ONLY when the digest still matches. A summarized / capped / rewritten body fails the check, so the gate re-delivers the full contract (fail-safe) instead of trusting a mutated one. XXH3 is fast and stable across processes (a resumed sub-agent's byte-identical history still verifies); non-cryptographic — it guards accidental mutation, not a forged collision. - new `xxhash-rust` dep (xxh3 feature) - `payload_digest` / `build_marker` assemble+verify the digest; the marker-to-body concatenation is owned by `deliver_body` and `prefix_with_present_marker` so the hashed bytes always equal the post-marker bytes - `present_marker(keys) -> Option<String>` becomes `prefix_with_present_marker(keys, body) -> String`; the three discovery pre-crediting call sites updated (mcp helper now returns keys, not a marker) - `collect_present_keys` parses `<digest>:<keys>` and drops the keys unless the recomputed digest matches - tests rebuilt on the real delivery path, plus new coverage: a truncated body and a rewritten body both fail the digest gate, and a digest-less marker is ignored Addresses the "preserve contract bodies through tool-output rewriting" review point on tinyhumansai#4861. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012BLvp9tiJ2bFUrcY821FY1
…contract leak) Codex flagged (tinyhumansai#4861 review) that the gate's `fetch_workflow` resolved a workflow contract straight from the global registry, while the real `run_workflow` / `describe_workflow` tools enforce the active profile's `skill_allowlist`. A model guessing a scoped-out `workflow_id` would receive that workflow's input contract from the gate — leaking metadata the profile hides. Thread the allowlist to the gate the same identity-from-the-tool way `composio_actions` already flows: `RunWorkflowTool` self-reports its `skill_allowlist` via a new `Tool::workflow_gate_allowlist` trait method, and `assemble_turn_harness` collects it into `ContractGateMiddleware`. `decide` then passes a scoped-out workflow straight through (no contract rendered) so the real tool owns the canonical "not available to the active agent profile" rejection — the gate never reveals such a workflow exists. Checked the other two gated surfaces: Composio (`composio_integrations`) and MCP-registry (`allowed_mcp_servers`) have NO equivalent leak, because their executing tools do not enforce that per-profile scope in the first place (the gate mirrors the absent enforcement, exposing nothing the model couldn't already reach by calling the tool). Only workflows enforce an allowlist the gate bypassed. - new default-None `Tool::workflow_gate_allowlist`; `RunWorkflowTool` overrides it - gate stores `workflow_allowlist`, checks it in `decide` via `workflow_allowed` - tests: scoped-out workflow not gated; allowed workflow still gated; a None allowlist gates every workflow Addresses the "honor workflow profile allowlists before returning contracts" review point on tinyhumansai#4861. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012BLvp9tiJ2bFUrcY821FY1
Reviewer tinyhumansai#1 on tinyhumansai#4861 asked for a test through the real `assemble_turn_harness` middleware ordering (not just unit-level `decide()`), exercising the whole deliver→retry so the transcript-derived presence check is validated end to end. - cfg(test)-only contract-source seam (`test_support::InjectedContracts`): `fetch_contract` first consults an injected map keyed by `GateTarget::key()`, so an integration test resolves a deterministic contract without reaching a live Composio / workflow / MCP source. The middleware is built inside `assemble_turn_harness`, so the test never holds it — the seam is global, serial-locked, and RAII-cleared. String-valued (delivered as `ContractResult::Found`) so the private `ContractResult` keeps its visibility. The whole module is `#[cfg(test)]` and absent from production builds. - integration test in `tinyagents::tests`: a mock provider issues the same gated `composio_execute` call twice (gated first attempt, then retry) with a recording stub tool, driven through `run_turn_via_tinyagents_shared` → `assemble_turn_harness`. The single assertion `executed == 1` proves BOTH halves: the gate blocked the first attempt (else 2) AND the delivered contract round-tripped through `before_model` so the retry passed (else 0, looping). Also asserts the delivery precedes the execution in the transcript. `openhuman::tinyagents` suite green (152, incl. the `adapter_inventory` middleware-count assertions — the seam adds no middleware); fmt clean. Addresses reviewer integration-test point tinyhumansai#1 on tinyhumansai#4861. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
…mansai#4872 clippy gate) Rebasing onto the merge that landed tinyhumansai#4872 (enforce warning-free clippy) surfaced two `clippy::write_with_newline` hits in `render_workflow_contract` — `write!` calls whose format string ends in a single `\n` (present since the first gate commit, harmless until the gate began failing on warnings). Switch them to `writeln!`. Byte-identical output; clears the `-D warnings` gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
The gate's contract resolvers reach into domain modules that upstream now compiles out under feature gates: Composio via `tinyflows::caps` (`flows`), workflows via `skills::registry` (`skills`), MCP via `mcp_registry` (`mcp`). The `--no-default-features` feature-gate smoke builds with all three off, so the unconditional references stopped compiling once this branch rebased onto that upstream (cannot find `tinyflows` / `get_workflow` / `WorkflowDefinition`). Gate each fetch/resolve/render path behind its domain feature; with a domain off the fetch degrades to `ContractResult::Unavailable` (pass-through) — semantically correct, since that domain's late-bound tools don't exist to gate. Shared render helpers (`pretty`, `join_capped`, the `fmt::Write` import) gate on the union of the three. Verified: `cargo check --no-default-features --features tokenjuice-treesitter` clean; default `cargo clippy --lib` warning-free; `openhuman::tinyagents` suite green (167, incl. the deliver→retry integration test + adapter_inventory counts). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
98515dd to
51aec00
Compare
…llowlist The feature-gate-smoke lane compiles AND runs the lib test binary with the domain gates off, so the contract-gate tests that name gated symbols (`resolve_composio`/`render_composio_contract`/`ToolContract` → `flows`, `resolve_mcp`/`render_mcp_contract` → `mcp`, `join_capped` → either) must carry the matching `#[cfg(feature = …)]` or the gates-off test binary fails to compile. Gate the five affected tests to match the resolvers they exercise. The lane's self-maintaining allowlist then flags the newly-gated file, so add `openhuman/tinyagents/contract_gate_tests.rs`. It also surfaced a latent gap the main lane hides (the guard is skipped on main pushes, runs only on PRs): `openhuman/composio/action_tool.rs` already carries a `#[cfg(feature = "flows")]` test from tinyhumansai#4995 but was never added — add it too so the guard is consistent. Verified gates-off: `cargo check` + `cargo test --lib` (compile + the lane's scoped run, 230 pass) clean; the guard allowlist == the grep's actual set. Default `openhuman::tinyagents::contract_gate` tests still green (33). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
…n the middleware) Upstream merged tinyhumansai#4995 ("gate per-action tools on their full contract"), a per-`ComposioActionTool` in-memory gate (`composio::contract_gate`). This PR's `tinyagents::contract_gate` middleware already gates per-action Composio calls — and more robustly: its "already surfaced" state is derived from the transcript with a payload-digest, so a contract summarised or compacted OUT of context is re-delivered. tinyhumansai#4995's own docs flag that its in-memory `seen`-set does NOT reset on compaction (the model then acts on a schema it can no longer see). To avoid double-gating (the middleware short-circuits the first call before execute, so tinyhumansai#4995's tool-layer consult would fire a SECOND contract on the retry and delay execution a turn), remove the tool-layer hookup: the `gate` field, its init, the `consult` call in `execute`, and its regression test. Changes to the merged code are kept minimal — `composio::contract_gate` itself is left in place (now unused in production, tracked for removal) and its unit tests still pass; only the per-action wiring in `action_tool.rs` (+ a stale lifecycle note in `provider.rs`) is touched. The middleware is now the single per-action gate. Verified: default `clippy --lib` clean, `openhuman::tinyagents::contract_gate` (33) + `composio::action_tool` (8) + dormant `composio::contract_gate` (3) green; gates-off `cargo check` + `cargo test --lib` compile clean; the feature-gate smoke allowlist matches (action_tool.rs drops its only gated test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
Rust Core Coverage failed on a pre-existing test-isolation flake in `learning::startup` (its own tests pollute a global subscriber registry — a different one fails per run; tinyhumansai#5003 area). This PR does not touch `learning/`. Empty commit to re-run CI; squashed away at merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
Heads-up: the red
|
…batim) A sub-agent installs HandoffMiddleware, whose after_tool runs clean_tool_output — which collapses runs of whitespace to a single space, flattening the delivered contract's pretty-printed JSON-schema indentation. That changes the bytes after the [contract-gate:<digest>:<key>] marker, so the next before_model rescan re-hashes them and the digest no longer matches the one baked into the marker: the key is never credited and the contract is re-delivered every turn (an infinite loop, observed under the integrations sub-agent). It runs FIRST in the reverse-order after_tool chain, so the tool-output-budget guard couldn't protect it. Fix (openhuman-only): a general trusted_verbatim flag on ToolResult.raw. The gate marks a Found delivery with it, and both content-rewriting after_tool hooks (HandoffMiddleware, ToolOutputMiddleware) skip a flagged result — so the delivery reaches the transcript byte-for-byte and the digest matches on the retry. NotFound is NOT flagged (it carries no digest, and a looping bad slug should still trip the repeated-tool-failure breaker). Tests: clean_tool_output_collapses_indentation_whitespace (documents the hazard), trusted_verbatim_flag_round_trips_and_survives_credential_scrubbing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
Follow-up (
|
The 37efade tests weren't run through rustfmt: wrap the long JSON string literal in handoff.rs and the assert! in middleware.rs to satisfy cargo fmt --check (the Rust Quality + Frontend Checks lanes both run it). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
Note on discovery pre-crediting under sub-agent handoff cleaning (re: @M3gA-Mind's latency point #3)Two related clarifications on the "well-behaved path pays the round-trip" observation and on the 1. The discovery pre-credit path already avoids the round-trip in native mode. The full-schema discovery tools — 2. That pre-credit is verified by the same digest as the gate's own delivery, i.e. 3. Known limitation (best-effort, not a loop): discovery outputs are not flagged
Why not just flag discovery outputs |
Presence was tracked by embedding an XXH3 digest of the payload inside the
transcript marker (`[contract-gate:<digest>:<keys>]`) and re-hashing on rescan.
Switch to a process-global `DELIVERED: HashMap<slug-list, payload-hash>`: the
marker now carries ONLY the slug list (`[contract-gate:<slug list>]`, no
digest), and `refresh_present` credits a marker's slugs iff its trailing payload
still hashes to the value recorded for that exact slug list at delivery.
Same integrity guarantee — a contract summarized, size-capped, or
whitespace-collapsed by the sub-agent handoff cleaner no longer hash-matches and
is re-delivered — but the marker stays short and whitespace-free, and this is the
mechanism the combined/flo-tttf-gate branch converges on, so landing that branch
later re-commits no same-role gate code.
- deliver_body returns (body, payload_hash); wrap_tool and
prefix_with_present_marker record it in DELIVERED on first delivery.
- Removed collect_present_keys / build_marker / payload_digest /
MARKER_DIGEST_SEP; added credit_marker / normalize_slug_list /
contract_marker / payload_hash.
- test_support::{clear_delivered, isolate_delivered} isolate the global map
across the (serial) presence tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
Summary
ContractGateMiddleware: the first call each turn to a late-bound tool — the genericcomposio_executedispatcher, a per-action Composio tool, an MCP-registry remote tool, or a Workflow — is short-circuited with that target's full contract as a tool error, so the retry runs with the real schema in context.GMAIL_LIST_MESSAGESin place ofGMAIL_FETCH_EMAILS.[contract-gate:<digest>:<key>]marker — so it is re-surfaced after summarization / microcompact / hard-trim, and not needlessly re-delivered when it survives a resumed durable sub-agent'sinitial_history.OPENHUMAN_CONTRACT_GATE=0.Relationship to #4995
#4995 (which closed #4853) gates per-action Composio tools at the tool layer (
composio::contract_gate, an in-memoryseen-set perComposioActionTool). This PR unifies and extends that:composio_executedispatcher, the MCP-registry bridge, and workflows.seen-set does not reset when the surfaced schema drops out of context. This gate derives presence from the transcript + a payload digest, so a compacted/summarized/rewritten contract is re-delivered.gatefield, its init, theconsultcall inComposioActionTool::execute, and its regression test) is removed. Changes to the merged code are kept minimal —composio::contract_gateitself is left in place (now unused in production, its unit tests still green, removal tracked as follow-up).Problem
Several tool surfaces expose only a thin representation to the model while the full contract lives behind a separate discovery/describe meta-tool:
composio_executedispatcher has an opaquearguments: {"type":"object"}, and per-actionComposioActionTools are built from a one-linelist_toolsdescription (parametersmay beNone), not the fullfetch_live_toolkit_catalog.mcp_registry_tool_calltakes opaquearguments; the realinput_schemaonly appears inmcp_registry_list_tools.run_workflowtakes opaqueinputs; the declared inputs live behinddescribe_workflow.So the model composes a call before the real schema is in its context and guesses.
Solution
ContractGateMiddleware(src/openhuman/tinyagents/contract_gate.rs) is a dual-trait middleware:wrap_tool(the gate): resolve the call to aGateTarget(cheap, no I/O). If its contract marker is already in the transcript → run for real. Otherwise fetch the full contract and short-circuit with it (marked) as a tool error (Found), or return a not-found + valid list (NotFound); an unreachable source falls through (Unavailable).before_model(presence refresh): rescans the tool-role messages ofrequest.messagesfor[contract-gate:<digest>:<key>]markers and rebuilds the in-context set — correct by construction across resume, summarization, microcompact, and hard-trim. Scanning only tool-role messages means a model echoing the marker cannot spoof presence. The digest is re-hashed from the post-marker bytes and keys are credited only on a match, so a rewritten/truncated body re-delivers.Registered in
assemble_turn_harnessas one sharedArcin two lists:push_middlewareafter the compaction/trim middlewares, andpush_tool_middlewareright afterschema_guardand before approval/policy.Feature-gate safe: the Composio / workflow / MCP contract sources live in domain modules gated by
flows/skills/mcp. Each fetch/resolve/render path is#[cfg]-gated to match; with a domain compiled out the gate degrades to pass-through. The--no-default-featuresfeature-gate-smoke lane (lib check + gates-offcargo test) is green.Contract sources reuse existing code:
fetch_live_toolkit_catalog(Composio),all_connected_tools(MCP registry),get_workflow(Workflows).Deliberately scoped out (follow-up): the MCP config/legacy bridge (
mcp_call_tool) — its registry handle is held privately with no ambient accessor.Submission Checklist
contract_gate_delivers_the_contract_then_the_retry_executes) driving a gatedcomposio_executecall through the realassemble_turn_harnessmiddleware ordering viarun_turn_via_tinyagents_shared— the single assertionexecuted == 1proves both halves of deliver→retry end to end.N/A: internal agent-harness middleware; no user-facing feature row.xxhash-rust(XXH3) crate for the digest only.Closes #5038in## Related.Impact
OPENHUMAN_CONTRACT_GATE=0→ byte-identical behavior.Related
mcp_call_tool); remove the now-unusedcomposio::contract_gatemodule.Validation Run
feat/tool-contract-gate— Commit SHA:3e376622c(rebased onto latestupstream/main).cargo test --lib -- openhuman::tinyagents::contract_gate→ green; the deliver→retry harness integration test green.cargo check --no-default-features --features tokenjuice-treesitter+ gates-offcargo test --libcompile — green; the lane's gated-test allowlist matches.cargo fmtclean; defaultcargo clippy --libwarning-free (satisfies the chore(rust): enforce warning-free clippy #4872 warning-free-clippy gate).