Consume focused TinyInference crates and own prompt-guided providers - #152
Conversation
Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsReview configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (216)
Comment |
Co-authored-by: Medulla <medulla@tinyhumans.ai>
senamakel
changed the title
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
…tinyinference # Conflicts: # crates/tinyagents-harness/Cargo.toml
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
senamakel
changed the title
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 22 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Changes requested Review snapshot
Completeness: Complete 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
Before merge
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
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: 22230c5dbf
ℹ️ 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".
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: 805d64fa25
ℹ️ 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".
Co-authored-by: Medulla <medulla@tinyhumans.ai>
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: 063ec0ebda
ℹ️ 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".
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: 86a67c93ee
ℹ️ 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".
| args.push("--budget".to_string()); | ||
| args.push(format!("{budget:.4}")); |
There was a problem hiding this comment.
When max_budget_usd is configured, every invocation adds --budget, but the Claude Code CLI reference documents the non-interactive spending limit as --max-budget-usd <amount>. The subprocess therefore rejects the optional budget configuration instead of enforcing it; pass the documented flag and cover the configured invocation with a process-level test.
Useful? React with 👍 / 👎.
| let messages = coalesce_prompt_tool_results(&request.messages); | ||
| let messages = with_prompt_tool_instructions(&messages, &request.tools); | ||
| let system = coalesce_system_prompt(&messages); |
There was a problem hiding this comment.
When a caller supplies ResponseFormat::JsonSchema or ResponseFormat::JsonObject, this adapter preprocesses messages and tools but never includes request.response_format in the CLI prompt. Explicit schema requests use the harness's provider-schema strategy, so an ordinary prose response then fails extraction even though the request asked for structured output. Add the corresponding JSON instruction before invoking the CLI and exercise both request shapes in a focused provider test.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| let schema = match format? { | ||
| ResponseFormat::JsonSchema { name, schema } | ResponseFormat::Auto { name, schema } => { | ||
| Some((name, schema)) | ||
| } | ||
| ResponseFormat::Text | ResponseFormat::JsonObject => None, | ||
| }?; |
There was a problem hiding this comment.
Fresh evidence after the earlier JSON-schema fix is that ResponseFormat::JsonObject is still explicitly mapped to no instruction. For direct callers and validation middleware requesting this format, Claude therefore receives no JSON-only constraint and can return prose that fails JSON parsing; emit a JSON-only instruction for this variant and add a focused request-shape test.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| if subtype.as_deref() == Some("error") && self.error.is_none() { | ||
| self.error = Some("claude reported `result.subtype=error`".into()); | ||
| } |
There was a problem hiding this comment.
When the CLI terminates a turn with an error result such as error_max_turns, error_during_execution, or another non-success subtype, this exact equality does not set mapper.error; the driver can consequently return an empty or partial successful response and persist the session. Parse the result's is_error field (already modeled by the sibling Agent SDK protocol), or treat every recognized non-success subtype as failure, and add an error-transcript test.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| anyhow::bail!( | ||
| "[claude-code][driver] exit {:?} stderr={}", | ||
| status.code(), | ||
| stderr_text.trim() | ||
| ); |
There was a problem hiding this comment.
When a previously persisted Claude session no longer exists—for example after the CLI's local session data is cleaned or moved—the resumed process exits nonzero here, but its thread mapping remains unchanged. Every retry for that thread consequently launches the same invalid --resume UUID and can never reach the new-session path; remove the stale mapping and retry once with a fresh --session-id when resume reports a missing session.
Useful? React with 👍 / 👎.
| let serialized = serde_json::to_string_pretty(&*guard).map_err(std::io::Error::other)?; | ||
| if let Some(parent) = self.path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
| std::fs::write(&self.path, serialized) |
There was a problem hiding this comment.
When two ClaudeCodeProvider instances use the same workspace—a normal setup when registering multiple Claude models—each opens an independent in-memory SessionStore. This write serializes only that instance's stale map and replaces the whole shared file, so provider B can erase mappings provider A just persisted; after restart those conversations lose resumability. Coordinate stores by path or reload and atomically merge under a shared/file lock before replacing the file.
Useful? React with 👍 / 👎.
| `--session-id` is used on a new CC session and `--resume` afterwards; the UUID | ||
| comes from `session_store.rs`, keyed by a SHA-256 hash of the conversation's | ||
| first user message (`thread_key_from_messages`) because the real OpenHuman | ||
| thread id is not yet plumbed through `ChatRequest`. `cwd` is |
There was a problem hiding this comment.
This module README still tells integrators that sessions are keyed by a hash of the first user message and that thread IDs are unavailable, while thread_key_from_request now requires caller-provided metadata and deliberately creates a fresh ephemeral key otherwise. A host following this documentation will omit metadata.thread_id and silently lose session continuity on every turn; update this section and the stale file-map references to match the public adapter behavior.
AGENTS.md reference: AGENTS.md:L76-L80
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: 4 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.0507 · 1,243,905 in / 63,876 out · 206,212 cached (17%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 1,198 embedded
critique: $0.0236 · 448,556 in / 33,737 out · 52,079 cached (12%) · gpt-5.6-luna, deepseek-v4-flash
security: $0.0212 · 392,251 in / 13,573 out · 14,869 cached (4%) · gpt-5.6-luna
tests: $0.0035 · 278,899 in / 10,713 out · 138,240 cached (50%) · deepseek-v4-flash
description: $0.0025 · 124,199 in / 5,853 out · 1,024 cached (1%) · deepseek-v4-flash
| use crate::tool::{coalesce_prompt_tool_results, with_prompt_tool_instructions}; | ||
| use anyhow::Context; | ||
| use async_trait::async_trait; | ||
| use tinyinference_llm::message::Message; |
There was a problem hiding this comment.
Declare the tinyinference_llm dependency
The reviewed repository has no Cargo manifest declaring tinyinference_llm, so this new provider cannot compile. Add the package dependency under the declared crate name, or import the inference crate that the workspace actually declares.
[RULE] undeclared-dependency ·
| } | ||
|
|
||
| /// Prompt-guided chat model backed by `claude -p`. | ||
| pub struct ClaudeAgentSdkProvider { |
There was a problem hiding this comment.
Declare the tinyinference_llm dependency
This file imports tinyinference_llm, but the reviewed repository does not declare that dependency name in its Cargo manifests. The provider therefore cannot compile until the package dependency is declared under that name or the import is changed to the crate name the workspace actually provides.
[RULE] declared-dependency ·
| } | ||
|
|
||
| fn model_response(response: ChatResponse) -> ModelResponse { | ||
| let usage = response.usage.map(|value| Usage { |
There was a problem hiding this comment.
Clone usage before reusing the response field
Option::map takes ownership of response.usage. model_response then reads response.usage again when constructing raw, so this new module does not compile unless the usage field is unexpectedly Copy (the contained usage value is not shown as such here). Clone the option or compute the raw value before moving the field.
| let usage = response.usage.map(|value| Usage { | |
| let usage = response.usage.clone().map(|value| Usage { |
[RULE] use-after-move ·
| let output = self | ||
| .invoke_cli(system.as_deref(), &transcript, model) | ||
| .await | ||
| .map_err(|error| tinyinference_llm::Error::Model(error.to_string()))?; |
There was a problem hiding this comment.
Preserve the retry classification API
Every subprocess failure is flattened into the generic Model error. This loses the structured provider/transport classification and retryability information expected by the harness retry policy, so transient CLI or transport failures will not be retried according to policy. Convert the error into the inference crate's structured provider error while preserving retryability and any stable error code.
[RULE] error-classification-loss ·
| tinyinference_llm::ProviderFailureClass::NonRetryable | ||
| | tinyinference_llm::ProviderFailureClass::NonRetryableRateLimit | ||
| ) { | ||
| tinyinference_llm::Error::Model(message) |
There was a problem hiding this comment.
Preserve the provider retry classification
This converts every failure that is not classified as a non-retryable error into the generic Error::Model, and converts the remaining cases into Error::Validation. That discards the structured provider classification, including rate-limit and transient categories, so the harness retry policy cannot reliably distinguish retryable failures from validation failures. Return an error variant carrying the classification (or the retryable flag/code) instead of collapsing it to these two generic variants.
[RULE] preserve-retry-classification ·
| max_budget_usd: Option<f64>, | ||
| ) -> ClaudeInvocation { | ||
| let stdin = match system_prompt { | ||
| Some(system) if !system.trim().is_empty() => { |
There was a problem hiding this comment.
Pass system instructions through the CLI system-prompt option
System instructions are embedded in ordinary stdin text using delimiters instead of being passed through Claude's dedicated system-prompt option. This makes the system content part of the user-visible transcript and allows surrounding prompt content to affect how those delimiters are interpreted, weakening the system-instruction boundary. Add --system-prompt and its value to the CLI arguments and keep the request body as the non-system transcript.
[RULE] ignored-provider-option ·
| // Replaying a cached first turn would skip those side effects entirely, | ||
| // so this provider is never response-cacheable. Other providers retain | ||
| // the normal request-policy behavior. | ||
| let side_effecting_provider = binding |
There was a problem hiding this comment.
Exclude claude-agent-sdk provider from response caching
The response cache decision block only disables caching for claude-code. The claude-agent-sdk provider (ClaudeAgentSdkProvider) also executes file and shell tools inside the subprocess turn; replaying its cached response would skip those side effects entirely. Add "claude-agent-sdk" to the condition so caching is also disabled for that provider.
[RULE] missing-cache-exclusion ·
| .as_deref() | ||
| .or(self.profile.model.as_deref()) | ||
| .unwrap_or(&self.config.default_model); | ||
| let output = self |
There was a problem hiding this comment.
Classify subprocess errors before converting to Error::Model
Every failure from invoke_cli — spawn error, non-zero exit, timeout, CLI error message — is unconditionally mapped to Error::Model, which the harness treats as potentially retryable. Spawn failures (e.g., binary not found, permission denied) and non-zero exits are permanent and will cause infinite retries. Use the same map_error pattern as claude_code/mod.rs (call classify_provider_failure and return Error::Validation for non-retryable cases).
[RULE] unclassified-error ·
|
|
||
| /// Loads settings from the host-selected settings directory. A missing, | ||
| /// unreadable, or corrupt file yields safe defaults with full access disabled. | ||
| pub fn load(workspace_dir: &Path) -> ClaudeCodeSettings { |
There was a problem hiding this comment.
Reject symlinked settings before loading permissions
std::fs::read follows symlinks transparently. If an attacker can write a symlink at claude_code_settings.json pointing to a sensitive file, load will read it. Resolve the path to its real (canonical) path first and verify it is under an allowed directory. Use std::fs::canonicalize and check it's within the expected workspace. Same issue applies to save at the std::fs::write call.
[RULE] symlink-following ·
| std::fs::create_dir_all(parent)?; | ||
| } | ||
| let json = serde_json::to_vec_pretty(settings).map_err(std::io::Error::other)?; | ||
| std::fs::write(&path, json)?; |
There was a problem hiding this comment.
Prevent settings writes from following symlinks
std::fs::write follows symlinks. If an attacker placed a symlink at claude_code_settings.json pointing outside the workspace, the write would escape. Canonicalize the path and verify it is under the intended workspace before writing.
[RULE] symlink-following ·
tinysweeper
Bot
added
the
priority: p0
1 participant
Summary
tinyinference-llm.tinyinference-embeddings.Depends on tinyhumansai/tinyinference#12. This PR remains draft until that dependency lands and the gitlink is refreshed to its merged commit.
Validation