feat(harness): media feature with GenerateImageTool / GenerateVideoTool - #208
Conversation
The tinyinference submodule is updated to a newer commit, including local uncommitted changes. This keeps the vendored dependency in sync with the upstream repository. Auto-committed-on: macbook
The Display implementation for media types was inadvertently removed during a refactor, causing compilation errors for code that relied on formatting these types. This change restores the trait implementation to its previous behavior. Auto-committed-on: macbook
Introduce a new image tool module in the tinyagents-harness crate to support media-related image operations, enabling agents to handle image inputs and outputs within the harness framework. Auto-committed-on: macbook
The video tool implementation was previously deleted, and this change restores it to the media module. The tool is needed again for handling video inputs in the harness, so the original functionality is brought back intact. Auto-committed-on: macbook
When the media directory does not exist, the harness now creates it automatically instead of panicking. This improves robustness when running tests in clean environments where the directory has not been pre-created. Auto-committed-on: macbook
Adds a test file for the media module to verify its behavior. This provides initial coverage for the media-related functionality in the harness crate. Auto-committed-on: macbook
Add an optional `media` feature to the harness that enables image and video generation capabilities through the new `tinyinference-image` and `tinyinference-video` crates. This includes a new `media` module, re-exports of the underlying crates, and error conversion implementations that map their errors into the existing `TinyAgentsError` hierarchy. Auto-committed-on: macbook
…nyagents-harness/src/lib.rs,cra Auto-committed-on: macbook
Replace the pattern of creating a default VideoRequest and then mutating each field individually with a struct literal that sets all fields at once, using the spread operator to fill in any remaining defaults. This makes the construction more idiomatic and reduces the number of mutable assignments. Auto-committed-on: macbook
Adds documentation for the harness media module, covering its purpose and usage. Auto-committed-on: macbook
Add links to the new media generation documentation in both the harness module README and the spec README, so the feature is discoverable from the main documentation indexes. Auto-committed-on: macbook
…urable The image and video generation tools now accept overrides for their permission level and tool category, defaulting to write permission and the system category. This allows hosts to gate these tools differently depending on the calling context. The tinyinference submodule is also updated to a newer revision. Auto-committed-on: macbook
Updated the pinned commit of the vendor/tinyinference subproject to include recent changes, with a dirty suffix indicating uncommitted modifications in the submodule. Auto-committed-on: macbook
Updated the pinned commit for the tinytools vendored dependency to incorporate upstream fixes and improvements. Auto-committed-on: macbook
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
✨ Finishing Touches📝 Generate docstrings
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: b1e5b648fb
ℹ️ 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".
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 10 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
Previously reported and still active
Resolved this pass
Could not review: tinysweeper/description, tinysweeper/tests Before merge
How this fits togetherflowchart LR
n0["TinyAgentsError<br/>changed<br/>1 finding"]:::blocking
n1["from_provider_error"]:::impacted
n2["run_phase"]:::impacted
n3["map_reduce"]:::impacted
n4["is_context_overflow"]:::impacted
n5["providers"]:::impacted
n6["openai"]:::impacted
n1 -->|uses| n5
n1 -->|uses| n6
n2 -->|uses| n0
n2 -->|calls| n3
n3 -->|uses| n0
n4 -->|uses| n5
n4 -->|uses| n6
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
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is high.
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.0543 · 1,038,090 in / 44,953 out · 43,896 cached (4%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash, deepseek-v4-flash · 1,044 embedded
critique: $0.0315 · 627,077 in / 22,090 out · 31,386 cached (5%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0158 · 339,228 in / 7,869 out · 10,718 cached (3%) · gpt-5.6-luna
tests: $0.0041 · 30,739 in / 8,046 out · 1,792 cached (6%) · deepseek/deepseek-v4-flash
description: $0.0005 · 21,898 in / 3,904 out · 0 cached (0%) · deepseek-v4-flash
tinysweeper
Bot
added
the
priority: p1
When deserializing media types from JSON, the code now correctly handles optional fields that may be absent from the input, preventing deserialization failures for valid payloads that omit these fields. Auto-committed-on: macbook
When deserializing media types from JSON, the code now correctly handles optional fields that may be absent from the input, preventing deserialization failures for valid payloads that omit non-required attributes. Auto-committed-on: macbook
When deserializing media types from JSON, the code now correctly handles optional fields that may be absent from the input, preventing deserialization failures for valid media objects that omit non-required properties. Auto-committed-on: macbook
When the image tool is called with a file path that does not exist, it now returns a descriptive error instead of panicking or silently failing. This improves robustness and debuggability for users providing invalid image paths. Auto-committed-on: macbook
When the image tool returns a result without a file path, the system now gracefully handles the missing value instead of panicking. This ensures robustness when the underlying image generation or retrieval process fails to produce a valid file. Auto-committed-on: macbook
The video_tool.rs file was missing from the tinyagents-harness crate, causing compilation failures when the media module was enabled. This change adds the file to restore the video tool functionality. Auto-committed-on: macbook
…configs Strengthen the workspace confinement logic so that symlinks targeting files outside the workspace are rejected, and tilde-prefixed references are treated as literal workspace paths rather than home-directory expansions. Also add validation to reject artifact subdirectories that escape the output root and integer values that exceed provider ranges, ensuring these errors are caught before billing occurs. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
… video responses Replaces the MockVideoGenerator in the empty_video_delivery test with a dedicated EmptyVideoGenerator that returns a VideoResponse with an empty videos list. This ensures the harness correctly treats empty video deliveries as billed, non-retryable errors, and tightens the assertion to check for the "do not retry" message and the request identifier. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
# Conflicts: # vendor/tinytools
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc57d93dc3
ℹ️ 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".
…for writes The artifact directory creation now inspects every path component without following symlinks, preventing a pre-existing symlink from redirecting output outside the workspace. The persist method uses O_NOFOLLOW on Unix and create_new to refuse writing through a final-path symlink or overwriting an existing file, ensuring that a scoped workspace cannot be escaped even after the directory check. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ff76e73f6
ℹ️ 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".
Update the tinyinference and tinytools submodule pointers to their latest commits, pulling in recent upstream changes. Auto-committed-on: macbook
Replace the previous O_NOFOLLOW-based file creation with an `openat` approach that holds an open file descriptor to the verified output directory. This prevents time-of-check/time-of-use races where an attacker could swap an ancestor directory after validation but before the write, redirecting the artifact to an unintended location. The `ArtifactDirectory` struct now carries the directory handle on Unix, and `persist` uses `openat` with `O_DIRECTORY | O_NOFOLLOW` to create files relative to that handle. Additionally, the video tool now derives the file extension from the media type header, supporting mp4, webm, and mov formats instead of hardcoding mp4. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a test that verifies the video tool saves media with the extension matching the content type from the provider, and add a WebmVideoGenerator stub to produce non-MP4 deliveries. The change also includes minor formatting adjustments in the media output module and adds the libc dependency to the lockfile. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…ions Replace direct libc system calls with the rustix crate to improve safety and portability. The change uses rustix's typed file descriptors and open flags instead of raw libc constants and unsafe blocks, reducing the risk of undefined behavior while maintaining the same security properties for artifact directory and file operations. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Update the pinned commits for the tinyinference and tinytools submodules to incorporate upstream fixes and improvements. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The non-unix file creation and write operations were not scoped within a block, causing the `file` variable to be used outside its intended scope. This change wraps them in a block to ensure proper scoping and correct control flow. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a8a55dd40
ℹ️ 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".
There was a problem hiding this comment.
Requesting changes: 3 lane(s) blocking, worst finding is high.
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.0484 · 738,337 in / 54,741 out · 83,720 cached (11%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash, deepseek-v4-flash · 1,069 embedded
critique: $0.0282 · 408,153 in / 25,798 out · 37,185 cached (9%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0147 · 236,279 in / 16,053 out · 17,863 cached (8%) · gpt-5.6-luna
tests: $0.0010 · 36,823 in / 5,549 out · 1,024 cached (3%) · deepseek-v4-flash
description: $0.0012 · 27,880 in / 3,827 out · 27,648 cached (99%) · deepseek/deepseek-v4-flash
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is high.
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.0817 · 1,306,915 in / 65,464 out · 64,888 cached (5%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 1,069 embedded
critique: $0.0442 · 710,247 in / 35,176 out · 34,524 cached (5%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0314 · 536,787 in / 26,240 out · 30,364 cached (6%) · gpt-5.6-luna
description: $0.0028 · 30,896 in / 624 out · 0 cached (0%) · deepseek/deepseek-v4-flash
…r resume Add input validation for string and string-list arguments in image and video tools to reject non-string values early. Also require the `model` parameter when resuming a video job, so the resumed result retains its submitted model. Additionally, validate that artifact name components are non-empty and safe filename components, and reject empty subdirectories in `MediaOutput::dir`. Auto-committed-on: dragonfly
…sertions The video tool's timeout policy now returns the configured wait timeout in milliseconds instead of being unbounded, ensuring the harness respects the tool's own deadline. The test for billed non-delivery was updated to match the actual error message, and the timeout policy assertion now expects the correct 5-second value. Auto-committed-on: dragonfly
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2df8633745
ℹ️ 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".
…ob handling The video tool's timeout policy was changed from a fixed millisecond duration to unbounded, allowing the provider's own wait budget to control job polling without interference from a generic harness timeout. This ensures that when a video job is still in progress, the tool returns a resumable timeout containing the job ID rather than a generic timeout that would lose the job context. Auto-committed-on: dragonfly
The `text_response` helper now constructs an `AssistantMessage` with an explicit `ContentBlock::Text` instead of relying on the deprecated `AssistantMessage::text` constructor. This aligns the test with the updated message API and ensures the helper continues to work after the old shorthand is removed. Auto-committed-on: dragonfly
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ac4ff222e
ℹ️ 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 handle = open( | ||
| &canonical_dir, | ||
| OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC, | ||
| Mode::empty(), |
There was a problem hiding this comment.
When another process can mutate a workspace containing a nested output subdirectory such as a/b, it can replace a with a symlink after canonical_dir is checked but before this pathname-based open. Fresh evidence in the final tree is that O_NOFOLLOW protects only the final b component, so the retained handle itself can point outside the workspace and the later handle-relative artifact write still escapes confinement. Walk from a retained root handle and open each component without following symlinks.
Useful? React with 👍 / 👎.
| match args.get(*key) { | ||
| None | Some(Value::Null) | Some(Value::String(_)) => {} | ||
| Some(other) => return Err(format!("`{key}` must be a string, got {other}")), |
There was a problem hiding this comment.
When a schema-valid call supplies a blank value such as "model": " ", this branch accepts it, but arg_str subsequently trims and discards it, so the image or video request is submitted and billed using the generator's default model rather than reporting invalid arguments. Require nonempty trimmed values for options consumed through arg_str, and cover this through the agent-loop invocation path.
AGENTS.md reference: AGENTS.md:L66-L70
Useful? React with 👍 / 👎.
| match args.get(*key) { | ||
| None | Some(Value::Null) | Some(Value::String(_)) => {} |
There was a problem hiding this comment.
When references is supplied as an empty or whitespace-only string, the model-facing schema accepts it and this branch declares it valid, while arg_list silently drops it. A reference-based edit can therefore become a separately billed text-only generation instead of failing before the provider call; apply the same nonempty check already used for array entries and add invocation coverage for the scalar form.
AGENTS.md reference: AGENTS.md:L66-L70
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is high.
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.0667 · 1,046,630 in / 49,208 out · 56,047 cached (5%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 1,081 embedded
critique: $0.0339 · 524,521 in / 24,804 out · 29,470 cached (6%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0262 · 456,935 in / 19,938 out · 26,577 cached (6%) · gpt-5.6-luna
description: $0.0032 · 33,579 in / 1,480 out · 0 cached (0%) · deepseek/deepseek-v4-flash
| "inputImages", | ||
| ], | ||
| ) { | ||
| match self.output.reference(&raw, workspace) { |
There was a problem hiding this comment.
Keep reference validation atomic with the provider read
The reference is validated and converted into a path before generate is called, so a local path can be swapped after validation (for example by replacing a symlink) and the provider can read a different target, including one outside the workspace. The validation result must bind the authorized bytes or otherwise keep the checked file identity/path invariant through the provider read; canonicalizing only during this call is not sufficient.
[RULE] atomic-path-validation ·
| )]; | ||
| for (index, image) in response.images.iter().enumerate() { | ||
| // Preserve the generated image format in the artifact extension | ||
| let ext = match image.media_type.split(';').next().map(str::trim) { |
There was a problem hiding this comment.
Allow only safe image extensions before persisting
The extension is selected from a small allowlist, which is safer than using the provider's raw media type, but the code still persists every response image before verifying that the response's data is non-empty or actually matches the declared format. A provider response with a supported type but invalid or empty bytes can create misleading artifacts and may fail after a billed call without a clear per-image validation. Validate the image payload and media-type contract before persistence, or make the persistence layer perform that validation.
[RULE] safe-artifact-extension ·
| &self.description | ||
| } | ||
|
|
||
| fn parameters_schema(&self) -> Value { |
There was a problem hiding this comment.
Add focused tests for image-tool boundary behavior
This change adds billing-sensitive behavior and runtime enforcement for empty responses, maximum image counts, malformed options, reference validation, and media-type handling, but the diff adds no tests. The repository rules require tests for every behavior change; add focused coverage for these branches so regressions cannot silently re-enable billed retries or unsafe persistence.
[RULE] missing-boundary-tests ·
|
|
||
| // Reject empty responses as billed failures, since the call was billed | ||
| // but produced nothing to save. | ||
| if response.images.is_empty() { |
There was a problem hiding this comment.
Add focused tests for image-tool boundary behavior
This change adds billing-sensitive behavior and runtime input enforcement, including empty responses, maximum image counts, malformed options, reference validation, and media-type handling, but the diff adds no tests. The repository rules require tests for every behavior change; add focused tests covering these branches so regressions cannot silently re-enable billed retries or unsafe persistence.
[RULE] missing-boundary-tests ·
| pub(crate) fn check_string_options(args: &Value, keys: &[&str]) -> Result<(), String> { | ||
| for key in keys { | ||
| match args.get(*key) { | ||
| None | Some(Value::Null) | Some(Value::String(_)) => {} |
There was a problem hiding this comment.
Reject empty string list arguments
This accepts an empty or whitespace-only single-string list value, such as {"references": ""}. arg_list then drops that value and the request proceeds as though the option was absent, which can silently change the requested operation and allow a billed request to run with no references. Apply the same non-empty check used for array entries to the single-string branch, or reject blank strings here.
Additional security observation
Reject empty string list values
[RULE] empty-input-validation
The single-string branch accepts an empty or whitespace-only string even though this helper promises that every list entry is a non-empty string. arg_list then silently drops that value, so a malformed references argument can be treated as absent and the media request may proceed and be billed instead of failing closed. Require the string to contain non-whitespace content so empty values reach the existing error branch.
Suggested change for this observation (reference only)
None | Some(Value::Null) => {}
Some(Value::String(text)) if !text.trim().is_empty() => {}
[RULE] silent-invalid-input ·
| let mut request = ImageRequest::new(prompt); | ||
| request.model = arg_str(args, &["model"]).map(str::to_owned); | ||
| // Enforce the maximum image count at runtime | ||
| if let Some(n) = arg_u64(args, &["n", "count"]) { |
There was a problem hiding this comment.
Reject image counts that cannot fit the request field
The runtime check only compares against MAX_IMAGES_PER_REQUEST. If that constant is larger than u32::MAX, or if the request field's representable range is narrower than the policy constant, u32::try_from(n).ok() silently turns an accepted value into None, causing the provider request to fall back to its default count. The conversion must be validated as part of the rejection condition rather than discarded.
[RULE] runtime-count-bound ·
| // comparison. If the root is a fallback path that doesn't exist, | ||
| // fall back to a lexical check after canonicalizing the path. | ||
| let canonical_root = root.canonicalize().unwrap_or_else(|_| root.to_path_buf()); | ||
| let admitted = match &self.reference_policy { |
There was a problem hiding this comment.
Keep reference validation atomic with the provider read
The path is canonicalized and admitted here, but only a path is returned; the provider reads that path later. A concurrent replacement of the file or an ancestor can therefore redirect the provider read after validation, including through a symlink changed after canonicalize. Validate and open the reference atomically, or pass an already-open safe handle through the provider boundary.
[RULE] toctou-path-validation ·
| ], | ||
| ) { | ||
| match self.output.reference(&raw, workspace) { | ||
| Ok(reference) => request.references.push(reference), |
There was a problem hiding this comment.
Keep reference validation atomic with the provider read
The reference is canonicalized and checked before being stored in the request, but the provider reads that path later inside generate. An attacker who can modify workspace filesystem entries can replace the validated file or introduce a symlink after validation and cause the provider to read outside the workspace. Pass an opened, immutable handle or the validated file bytes to the provider, or otherwise make validation and consumption atomic.
[RULE] filesystem-toctou ·
| extension: &str, | ||
| bytes: &[u8], | ||
| ) -> Result<PathBuf, String> { | ||
| if !is_filename_component(stem) || !is_filename_component(extension) { |
There was a problem hiding this comment.
Allow only safe image extensions before persisting
This check prevents traversal but accepts any single filename component as an extension, including active or browser-interpreted formats such as html, svg, or svg+xml. If artifacts are later served or opened by clients, generated bytes can become stored content with an executable or script-capable type. Restrict extensions at the image/video call boundary to an explicit allowlist of safe provider media formats before writing the artifact.
[RULE] unsafe-artifact-extension ·
| } | ||
| #[cfg(not(unix))] | ||
| { | ||
| let mut file = File::options() |
There was a problem hiding this comment.
Anchor non-Unix artifact writes to the verified directory
On non-Unix targets, validation and creation use the path again after the directory check. An attacker can replace the verified directory or an ancestor with a symlink between those operations, causing generated bytes to be written outside the output root. Use a platform-specific directory-handle-relative, no-follow creation primitive, or otherwise revalidate atomically before writing.
[RULE] path-race ·
tinysweeper
Bot
added
priority: p1
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is high.
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.0846 · 1,373,858 in / 64,255 out · 86,592 cached (6%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 1,081 embedded
critique: $0.0522 · 853,753 in / 35,129 out · 58,010 cached (7%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0290 · 488,654 in / 25,862 out · 28,582 cached (6%) · gpt-5.6-luna
| "inputImages", | ||
| ], | ||
| ) { | ||
| match self.output.reference(&raw, workspace) { |
There was a problem hiding this comment.
Keep reference validation atomic with provider reads
A local reference is validated and canonicalized here, but the resulting reference is then stored in request and read later by the provider. If an attacker can modify the workspace between these operations, they can replace a validated symlink or path component and make the provider read a file outside the workspace. Resolve the file into provider-owned bytes or otherwise hold an opened, validated handle through the provider read; canonicalizing only during validation does not close this race.
[RULE] toctou-path-validation ·
| pub(crate) fn check_string_lists(args: &Value, keys: &[&str]) -> Result<(), String> { | ||
| for key in keys { | ||
| match args.get(*key) { | ||
| None | Some(Value::Null) | Some(Value::String(_)) => {} |
There was a problem hiding this comment.
Reject empty string list values
check_string_lists accepts both an empty scalar string and an empty array: the scalar matches Value::String(_), and all(...) is true for an empty array. arg_list then drops these values, so {"references":""} and {"references":[]} are silently treated as if the option were absent. Reject blank scalar strings and empty arrays whenever the option is present.
[RULE] empty-input-validation ·
| @@ -0,0 +1,74 @@ | |||
| # Media generation tools | |||
|
|
|||
| Feature: `media` on `tinyagents-harness`. Module: `tinyagents_harness::media`. | |||
There was a problem hiding this comment.
Name the exposed harness feature correctly
The harness exposes multimodal, not media, as its Cargo feature. A user following this documentation cannot enable the module with --features media; the documented feature must be changed to multimodal (and the module remains tinyagents_harness::media).
| Feature: `media` on `tinyagents-harness`. Module: `tinyagents_harness::media`. | |
| Feature: `multimodal` on `tinyagents-harness`. Module: `tinyagents_harness::media`. |
[RULE] invalid-feature-name ·
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn malformed_string_options_are_rejected_not_ignored() { |
There was a problem hiding this comment.
Add direct coverage for image-tool boundary safeguards
This test file still does not exercise several externally visible image-tool safeguards through execute: the maximum image-count limit, unsupported response media types before persistence, non-string reference entries, or the direct reference/path rejection cases beyond traversal and symlinks. These checks are specifically billing- and filesystem-safety-sensitive, and schema assertions cannot replace runtime coverage because callers can invoke the tool directly. Add focused direct-execution tests for those branches before merging.
[RULE] missing-tests ·
| }) | ||
| } | ||
|
|
||
| /// A unique, filesystem-safe artifact stem: `<kind>-<unix-millis>-<counter>`. |
There was a problem hiding this comment.
Make artifact stems unique across processes
The counter only provides uniqueness within one process, and the timestamp has millisecond resolution. Two processes generating the same kind in the same shared workspace during the same millisecond both produce the same stem because each process starts its counter at zero. If those artifacts are persisted under that stem, one generation can overwrite the other. Use a process-independent unique identifier (for example, a UUID or sufficiently random component) instead of relying on the process-local counter.
[RULE] artifact-collision ·
| ToolTimeout::Millis(IMAGE_TIMEOUT_MS) | ||
| } | ||
|
|
||
| async fn execute(&self, args: Value) -> anyhow::Result<ToolResult> { |
There was a problem hiding this comment.
Add focused tests for image-tool boundary behavior
This change adds billing-sensitive and filesystem-sensitive guards, including maximum-count enforcement, empty billed responses, malformed options, media-type rejection, and reference confinement. The image tool's direct execute path bypasses model schema validation, but this diff adds no focused tests covering those boundaries or symlink replacement behavior. Add unit tests that exercise direct execution and assert requests are rejected before billing and that unsafe references cannot escape the workspace.
[RULE] missing-boundary-tests ·
| let result = tool.execute(json!({ "prompt": "x" })).await.unwrap(); | ||
| assert!(result.is_error); | ||
| let message = text(&result); | ||
| assert!( |
There was a problem hiding this comment.
Match the complete billed non-delivery error
This assertion only checks for two independent substrings, so a regression could change the request identifier, error classification, or wording while the test still passes. Assert the complete expected error text (or at least all contractually significant fields) so the billed non-retryable behavior is actually pinned.
[RULE] weak-test-assertion ·
| let result = tool.execute(json!({ "prompt": "x" })).await.unwrap(); | ||
| assert!(result.is_error); | ||
| let message = text(&result); | ||
| assert!( |
There was a problem hiding this comment.
Match the complete resumable-timeout error
The timeout regression test accepts any message containing the job id and a generic phrase. It does not verify the provider reason, timeout classification, or the exact resume/no-resubmit contract, so a materially changed error could pass unnoticed. Tighten this assertion to the actual expected error text or structured fields.
[RULE] weak-test-assertion ·
| canonical_dir.display() | ||
| )); | ||
| } | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
Anchor every verified artifact ancestor
The verified directory is opened through an absolute path after each ancestor was checked separately. A concurrent local attacker can replace an ancestor of canonical_dir with a symlink between those checks and open; NOFOLLOW only protects the final component, so the resulting directory handle can point outside the verified output root and openat will then safely write to the wrong directory. Traverse from an already-open root directory handle using directory-relative no-follow operations for every component, or otherwise bind the entire directory chain atomically.
[RULE] path-race ·
| // Canonicalize the root if it exists, for accurate symlink-safe | ||
| // comparison. If the root is a fallback path that doesn't exist, | ||
| // fall back to a lexical check after canonicalizing the path. | ||
| let canonical_root = root.canonicalize().unwrap_or_else(|_| root.to_path_buf()); |
There was a problem hiding this comment.
Fail closed when the reference root cannot be canonicalized
When canonicalizing root fails, this uses the uncanonicalized root for the confinement check while comparing it with a canonical reference path. That makes the authorization decision dependent on a lexical, unresolved path and can disagree with the actual filesystem location, especially for unusual or changing workspace roots. Return an error when the root cannot be canonicalized instead of falling back to root.to_path_buf().
[RULE] canonical-path-validation ·
1 participant
Summary
Adds a
mediafeature totinyagents-harnessthat exposes TinyInference's new generators (tinyhumansai/tinyinference#27) as ordinarytinytools::Tools:GenerateImageToolovertinyinference_image::ImageGenerator.GenerateVideoToolovertinyinference_video::VideoGenerator. It waits under a hostWaitPolicyand supportsresume_job_id, which collects a timed-out job without a second billed submit.MediaOutput: artifacts go into the run's workspace (generated-media/). Local reference paths are confined to the workspace (no.., nothing outside the root) unless the host installs its ownReferencePathPolicy.tinyinference_image/tinyinference_videoare re-exported when the feature is on, and both error types map intoTinyAgentsError.payment,network,external_serviceandwrites_files, plusidempotent: false, so a host never replays a billed call after a crash.The harness owns argument parsing (including the loose spellings and camelCase aliases models emit), artifact persistence and result wording. The host owns the generator, which carries the credential and endpoint.
Why
This is the harness half of fixing a billed non-delivery in OpenHuman: an image request failed three times and was charged each time. Each tool reports a billed non-delivery or timeout as an error that names the job and says not to retry.
Tests
crates/tinyagents-harness/src/media/test.rs(10), including:billed_non_delivery_tells_the_model_not_to_retryandvideo_timeout_names_the_job_and_resume_collects_it(resume submits nothing new).video_tool_waits_for_delivery_and_saves_the_clip, which polls throughcompletedwith no outputs.references_resolve_inside_the_workspace_and_are_confined_to_itanda_host_reference_policy_replaces_the_default_confinement.media_tools_declare_billing_side_effectsandmedia_errors_map_onto_harness_errors.Docs:
docs/modules/harness/media.md, linked from the harness README and the spec index.Verification
Merged latest
mainbefore pushing. Depends on tinyhumansai/tinyinference#27 (thevendor/tinyinferencegitlink points at that branch); move the gitlink to the merge commit once it lands.Summary by CodeRabbit