feat(harness): tool-result artifact persistence and use_skill tool packs (OpenHuman wave 2) - #239
Conversation
Introduce a new `tool_results` module within the artifacts crate to store and manage the outcomes of tool executions, enabling better tracking and retrieval of individual tool results during agent runs. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Fix the serialization of tool results to properly handle cases where tool outputs are empty, ensuring that the JSON representation includes the expected structure rather than omitting fields. This resolves a deserialization mismatch that occurred when processing tool results with no output content. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test assertion for tool results was updated to reflect a change in how the harness formats tool output, ensuring the test continues to validate the correct behavior. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Move the test module declaration from the artifacts mod.rs into tool_results.rs, placing it alongside the code it tests. This keeps the test module co-located with its subject and removes a separate test file inclusion from the parent module. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…ied redactor and tool names Moves OpenHuman's tool_result_artifacts mechanics into harness::artifacts::tool_results: ToolResultArtifactStore, apply_per_result_persistence, spill_aggregate_tool_results, artifact_read_target and page_artifact_read. The host supplies an ArtifactRedactor, the read/wrapper tool names and the largest body its reader opens. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Introduce a catalog module and associated types to enable structured discovery and registration of tool packs. This change provides a centralized way to list and access available packs, improving extensibility and maintainability of the tool system. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When a tool pack is not found, the harness now returns a clear error message instead of panicking. This improves robustness by allowing the system to report the issue and continue operating. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Make the `packs` module publicly accessible from the tool crate by adding a `pub mod packs` declaration, enabling external consumers to use pack-related functionality. Also reformat a method chain in `handle.rs` and collapse a format string in `render.rs` for improved readability, removing trailing newlines in the process. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Relaxed assertions that accepted partial matches are replaced with exact checks, ensuring the rendered schema and dispatched arguments are verified precisely rather than allowing any substring match. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
PackCatalog (host pack table + not-found marker), ToolPack, PackRegistryHandle, UseSkillTool and the listing/scoping renderers, lifted from OpenHuman's toolpacks. The pack table, group posture and registry binding stay with the host. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
…d path Remove the `use tinytools_agent::dialect::ToolOutcome` import and instead reference the type directly as `tinytools_agent::dialect::ToolOutcome` in both the production code and tests. This eliminates an unnecessary import while keeping the same behavior. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…uplicate root accessor Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Warning Review limit reached
This review includes 13 billable files and costs up to $3.25. Or wait 6 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36dd3a14c1
ℹ️ 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".
Replace the stale-session check with a recursive `newest_modified` function that walks the session tree without following symlinks, so nested tool writes keep their containing session alive. Add a canonical-path check in `persist` to reject artifact paths that escape the action directory via symlinks. Floor the per-result persistence budget at `MIN_ENVELOPE_ALLOWANCE_BYTES` to guarantee the recovery pointer is always written, even when the caller supplies a tiny budget. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The doc comment for the scope_use_skill_spec function used a `super::` prefix to reference `UseSkillTool::execute_with_context`, which is unnecessary since the type is already in scope. Removing the prefix keeps the documentation cleaner and avoids potential confusion about the path resolution. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 4 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Incomplete Review snapshot
Completeness: Incomplete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Could not review: crates/tinyagents-harness/src/artifacts/README.md, crates/tinyagents-harness/src/artifacts/mod.rs, crates/tinyagents-harness/src/artifacts/tool_results.rs, crates/tinyagents-harness/src/artifacts/tool_results_test.rs, crates/tinyagents-harness/src/tool/mod.rs, crates/tinyagents-harness/src/tool/packs/catalog.rs, crates/tinyagents-harness/src/tool/packs/handle.rs, crates/tinyagents-harness/src/tool/packs/mod.rs, crates/tinyagents-harness/src/tool/packs/render.rs, crates/tinyagents-harness/src/tool/packs/test.rs, crates/tinyagents-harness/src/tool/packs/tool.rs, crates/tinyagents-harness/src/tool/packs/types.rs Before merge
How this fits togetherflowchart LR
n0["tool"]:::impacted
n1["ToolDispatch"]:::impacted
n2["exposure"]:::impacted
n3["insert_dispatch"]:::impacted
n4["CanonicalDispatch"]:::impacted
n5["deferred_schemas_with_families"]:::impacted
n2 -->|calls| n0
n3 -->|uses| n1
n4 -->|implements| n1
n5 -->|calls| n0
n5 -->|uses| n0
n5 -->|calls| n2
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f82b6c798d
ℹ️ 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".
| let root = self.action_dir.join(ARTIFACT_ROOT); | ||
| let entries = match std::fs::read_dir(&root) { |
There was a problem hiding this comment.
When the model-writable artifacts/tool-results component is a symlink to a directory outside action_dir, read_dir follows it and each returned entry still reports as a normal directory; the later remove_dir_all(entry.path()) can therefore recursively delete an external directory once it is older than max_age. Canonicalize and confine the pruning root against the canonical action directory before enumerating or deleting anything.
Useful? React with 👍 / 👎.
| ); | ||
| } | ||
| } | ||
| tokio::fs::write(&absolute_path, sanitized.text.as_bytes()).await?; |
There was a problem hiding this comment.
The new parent-canonicalization check is fresh evidence that the previously reported escape was only partially fixed: it validates parent, but this write still follows a pre-existing symlink at the final call.txt component. If an action workspace already contains such a symlink for a predictable tool/call ID, persisting an oversized result overwrites its target outside the workspace; open the leaf with no-follow semantics or explicitly reject an existing symlink.
Useful? React with 👍 / 👎.
| let inner_args = args.get("args").cloned().unwrap_or_else(|| json!({})); | ||
| tracing::debug!(tool = tools[idx].name(), "[toolpacks] use_skill dispatch"); | ||
| tools[idx] | ||
| .execute_with_context(inner_args, options, context) | ||
| .await |
There was a problem hiding this comment.
When a packed tool declares a host- or call-ID-injected argument, harness admission only prepares the outer use_skill declaration, whose injected-argument list is empty, and these nested model-supplied args are then forwarded directly. Consequently required call IDs are absent and model-provided values for host-authoritative fields are never stripped or replaced, allowing forged authority for any such packed tool; route the inner invocation through canonical argument preparation rather than calling it directly.
Useful? React with 👍 / 👎.
| tools[idx] | ||
| .execute_with_context(inner_args, options, context) | ||
| .await |
There was a problem hiding this comment.
When a packed tool uses ToolPolicy to require approval, a sandbox, or side-effect restrictions, the agent loop evaluates only the outer use_skill policy before reaching this direct call. UseSkillTool retains the default policy, so the inner tool can execute without the approval deferral and policy middleware behavior it receives when invoked normally; inner dispatch must preserve the canonical policy-admission path rather than forwarding only PermissionLevel.
Useful? React with 👍 / 👎.
| let persisted_output = if looks_like_preview_envelope(&original) { | ||
| Ok(PersistedToolResult { | ||
| output: original.clone(), | ||
| path: "<existing-preview>".to_string(), | ||
| original_bytes: original_len, | ||
| stored_bytes: original_len, | ||
| redacted: false, | ||
| }) |
There was a problem hiding this comment.
If an ordinary tool result happens to begin with [tool_result_preview]\n, this prefix check treats it as an already-persisted envelope even though no artifact exists. During aggregate spilling the result is then truncated in place instead of being written, permanently discarding its tail and potentially leaving a spoofed pointer; carry persistence metadata alongside the result or validate a real store-generated artifact rather than classifying untrusted output by prefix.
Useful? React with 👍 / 👎.
| original_bytes: {}\n\ | ||
| stored_bytes: {}\n\ | ||
| artifact_path: {relative_path}\n\ | ||
| read_with: {read_tool} {{\"path\":\"{relative_path}\"}} (a long read returns one page and names the \"offset\" to continue from)\n\ | ||
| notes: Full scrubbed output was persisted under the action workspace.{redaction_note}{truncation_note}\n\n\ | ||
| [preview]\n{preview}", | ||
| content.len(), | ||
| sanitized.text.len(), |
There was a problem hiding this comment.
When full_output exceeds max_readable_bytes but the earlier rewritten content fits, readable_body selects that fallback, yet this envelope still says the full output was persisted and reports the fallback length as original_bytes. The model and artifact index therefore claim recoverability and fidelity that no longer exist; propagate whether fallback selection occurred and label the stored body and byte counts accordingly.
Useful? React with 👍 / 👎.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0104 · 127,459 in / 14,083 out · 50,688 cached (40%) · ladder/vectors, deepseek/deepseek-v4-flash · 1,110 embedded
tests: $0.0016 · 48,550 in / 4,037 out · 48,384 cached (100%) · deepseek/deepseek-v4-flash
description: $0.0047 · 39,462 in / 7,254 out · 2,304 cached (6%) · deepseek/deepseek-v4-flash
|
|
||
| impl ArtifactRedactor for TestRedactor { | ||
| fn redact(&self, content: &str) -> Redacted { | ||
| let mut out = content.replace("ghp_abcdefghijklmnopqrstuvwxyz123456", "[REDACTED_SECRET]"); |
There was a problem hiding this comment.
Remove the committed credential (a GitHub personal access token)
Rotate the credential first, then remove it from the working tree and purge it from history. Treat it as compromised: it is in the push, whatever happens next.
Matched: ghp_… <redacted, 36 chars>
[RULE] github-personal-access-token ·
| let raw = format!( | ||
| "{} {}", | ||
| "x".repeat(4096), | ||
| "ghp_abcdefghijklmnopqrstuvwxyz123456" |
There was a problem hiding this comment.
Remove the committed credential (a GitHub personal access token)
Rotate the credential first, then remove it from the working tree and purge it from history. Treat it as compromised: it is in the push, whatever happens next.
Matched: ghp_… <redacted, 36 chars>
[RULE] github-personal-access-token ·
| assert!(out.contains("original_bytes:")); | ||
| assert!(out.contains("[preview]")); | ||
| assert!(out.contains("Credential/PII redaction was applied")); | ||
| assert!(!out.contains("ghp_abcdefghijklmnopqrstuvwxyz123456")); |
There was a problem hiding this comment.
Remove the committed credential (a GitHub personal access token)
Rotate the credential first, then remove it from the working tree and purge it from history. Treat it as compromised: it is in the push, whatever happens next.
Matched: ghp_… <redacted, 36 chars>
[RULE] github-personal-access-token ·
| ) | ||
| .unwrap(); | ||
| assert!(stored.contains("xxxx")); | ||
| assert!(!stored.contains("ghp_abcdefghijklmnopqrstuvwxyz123456")); |
There was a problem hiding this comment.
Remove the committed credential (a GitHub personal access token)
Rotate the credential first, then remove it from the working tree and purge it from history. Treat it as compromised: it is in the push, whatever happens next.
Matched: ghp_… <redacted, 36 chars>
[RULE] github-personal-access-token ·
tinysweeper
Bot
added
the
priority: p0
1 participant
Two host-independent mechanisms lifted out of OpenHuman's core. Both reach the host through a small seam; the product data and policy stay in the host.
1.
artifacts::tool_results(per-tool-result persistence)ToolResultArtifactStore,apply_per_result_persistence,spill_aggregate_tool_results,artifact_read_target,page_artifact_read. Extends the existingharness::artifacts(reusesArtifactRedactor/Redacted, no second policy trait).Seams: the host passes an
Arc<dyn ArtifactRedactor>, its file-read tool name, the wrapper tool name (use_skill) and the largest body its reader opens. The envelope and page trailer text is byte-identical (literal fixture testenvelope_text_is_byte_stable). The random fallback file name no longer needsuuid.2.
tool::packs(on-demand tool disclosure,use_skill)PackCatalog(host pack table + not-found marker),ToolPack,PackRegistryHandle,UseSkillTool,render_pack_filtered,scope_use_skill_spec,route_sentence,NoSuchPackTool,named_tool. Tool name, description and schema are pinned by a literal test. Distinct fromtool::discover(model-driven BM25 search over deferred schemas).The host keeps the pack table, group posture (
ToolGroups) and registry binding.3. Gitlink
Bumps nested
vendor/tinyinferenceto tinyhumansai/tinyinference (oh-w2-business-limit), which exposescontains_business_limit. Merge that PR first.Stacking
Based on
main. The OpenHuman pin (ed9cc77, from #230) is already an ancestor ofmain; 15 commits on top.Tests
cargo test -p tinyagents-harness --lib artifacts::tool_results: 21 passed;tool::packs: 16 passed. clippy -D warnings and fmt clean.