Repository navigation
Conversation
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 12 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
Resolved this pass
Before merge
How this fits togetherflowchart LR
n0["...minal_stream_item_disarms_its_abort_guard<br/>changed"]:::changed
n1["new"]:::impacted
n2["ModelResponse"]:::impacted
n3["ModelStreamItem"]:::impacted
n4["ChatModel"]:::impacted
n5["stream"]:::impacted
n6["ingest"]:::impacted
n0 -->|calls| n1
n0 -->|tests| n1
n0 -->|uses| n5
n1 -->|uses| n2
n1 -->|uses| n3
n3 -->|uses| n2
n4 -->|uses| n2
n5 -->|calls| n1
n6 -->|uses| n3
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
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Comment |
|
@senamakel pls review |
![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 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.0731 · 1,237,432 in / 56,882 out · 96,922 cached (8%) · gpt-5.6-luna, glm-5.3-flash, , gpt-6-luna
critique: $0.0480 · 673,614 in / 36,421 out · 61,146 cached (9%) · gpt-5.6-luna, glm-5.3-flash,
security: $0.0200 · 292,907 in / 14,956 out · 35,776 cached (12%) · gpt-5.6-luna
tests: $0.0008 · 87,709 in / 1,465 out · 0 cached (0%) · glm-5.3-flash
description: $0.0008 · 87,349 in / 80 out · 0 cached (0%) · glm-5.3-flash
| ) -> Result<ModelResponse> { | ||
| self.validate_handle(handle)?; | ||
| let outgoing = | ||
| self.inner |
There was a problem hiding this comment.
Use the configured endpoint when retrieving responses
When responses_alias is enabled, submission uses create_path() and therefore targets /responses, but all lifecycle retrieval paths here hard-code /agent. A handle created under that configuration can consequently be submitted successfully and then retrieved, resumed, cancelled, or have files accessed through the wrong endpoint. Derive the collection segment from the same configuration used for creation, or store the endpoint in the handle metadata and validate/use it consistently.
[RULE] endpoint-mismatch ·
| /// Returns the default provider spec for a known provider. | ||
| pub fn for_kind(kind: ProviderKind) -> Self { | ||
| match kind { | ||
| ProviderKind::Perplexity => Self::new( |
There was a problem hiding this comment.
Provide a valid default Perplexity model
ProviderSpec::model is documented as the default provider model id, but the new Perplexity default sets it to an empty string. A caller using ProviderSpec::for_kind(ProviderKind::Perplexity) therefore receives a configuration with no model and may send an invalid request or fail before making a request. Use the adapter's documented default model (for example, a supported sonar model), or verify that the adapter intentionally fills this value before the request; the adapter implementation was not included in the reviewed context.
[RULE] invalid-default ·
| }; | ||
| Ok(ModelResponse { | ||
| output: self.output.into_values().collect(), | ||
| execution: self.execution.map(|mut execution| { |
There was a problem hiding this comment.
Preserve execution progress in reconstructed responses
When a stream has an Execution event whose ModelExecution.progress is already populated but has no separate Progress events, this assignment replaces that progress with the empty accumulator vector. The completed-response path explicitly preserves existing progress and only backfills when it is empty, so the no-Completed path should apply the same rule.
Additional security observation
Preserve execution progress during reconstruction
[RULE] preserve-stream-metadata
When no Completed item is received, this unconditionally replaces progress already present on execution with self.progress. If progress was supplied in the execution event but no separate Progress events were emitted, the reconstructed response silently loses it. Only backfill progress when the execution's existing progress is empty, matching the handling used for completed responses.
Suggested change for this observation (reference only)
execution: self.execution.map(|mut execution| {
if execution.progress.is_empty() {
execution.progress = self.progress;
}
execution
}),
[RULE] preserve-stream-state ·
| let outgoing = | ||
| self.inner | ||
| .request(reqwest::Method::GET, &path(&["agent", &handle.id])?, None)?; | ||
| let incoming = self.inner.send(outgoing, Operation::Read, deadline).await?; |
There was a problem hiding this comment.
Guard every deferred request against network denial
submit_background checks ensure_network_models_allowed, but retrieval, resume, cancellation, file listing, and file download issue network requests without that check. After a caller creates a handle, calling deny_network_models() does not prevent these later requests, contrary to the repository contract that every request-issuing path calls the guard. Add the guard to each public deferred-operation entry point (or otherwise ensure every individual request is guarded).
[RULE] network-guard ·
| /// # Errors | ||
| /// Returns a serialization error if options cannot be encoded. | ||
| pub fn apply_to(&self, request: &mut ModelRequest) -> Result<()> { | ||
| super::request::validate_options_before_serialization(self)?; |
There was a problem hiding this comment.
Validate every hosted tool before serialization
The validation invoked here only checks PerplexityTool::WebSearch; ImageSearch.max_results and FetchUrl.max_urls are not validated. For example, ImageSearch { max_results: Some(0), .. } and FetchUrl { max_urls: Some(11) } are serialized successfully even though their documented ranges are 1–30 and 1–10. Reject these values before writing provider_options, otherwise callers can reach the provider with invalid requests and receive avoidable request failures.
[RULE] missing-validation ·
| crate::failure::classify_provider_failure(None, code.as_deref(), &message) | ||
| .is_retryable(); | ||
| return Err(Error::Provider(Box::new(ProviderError { | ||
| partial_response: None, |
There was a problem hiding this comment.
Populate the partial response on stream failure
When content has already been received and the stream later fails, provider_failure consumes the accumulator into a partial AssistantMessage but leaves ProviderError::partial_response as None. Callers therefore cannot access the accumulated usage, raw response, finish state, or the normalized full ModelResponse through the documented partial_response field; they only receive the legacy partial_message. Build the accumulated response once and assign it to partial_response while retaining its message for partial_message.
[RULE] preserve-partial-response ·
| if !functions.contains_key(name) { | ||
| return Err(invalid("selected function is not declared")); | ||
| } | ||
| } else if let Some(kind) = choice.get("type").and_then(Value::as_str) |
There was a problem hiding this comment.
Require forced hosted tools to be declared
A PerplexityToolChoice::Hosted("web_search") (or another allowed hosted kind) passes this validation even when options.tools contains no corresponding hosted tool. The resulting payload includes a forced tool_choice without declaring that tool, which the provider cannot honor and may reject. Validate that the selected hosted kind is present in the encoded hosted tools before inserting the choice.
[RULE] validate-tool-choice ·
There was a problem hiding this comment.
Resolved — the review agent found this finding fixed in the new code, as of 4ace3d3.
If this is wrong, reopen the conversation and say so; the finding will be re-raised on the next push if it still reproduces.
| pub struct ToolMessage { | ||
| /// Name and signed-call context for stateful native function results. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub call_context: Option<crate::tool::ToolResultContext>, |
There was a problem hiding this comment.
Update all ToolMessage literals for the new required field
Adding a field to this public struct breaks existing struct literals that construct ToolMessage without call_context (including production and test callers elsewhere in the crate). Rust requires every struct literal to specify the new field, so the crate will fail to compile as written. Update all callers in the same change, or redesign this API so adding context does not require every literal to change.
Additional critique observation
Preserve compatibility for existing ToolMessage literals
[RULE] breaking-api
Adding a field to this public struct requires every existing struct literal to initialize it. The repository still has ToolMessage { ... } literals that do not provide call_context, including production code, so the workspace will fail to compile; downstream callers using the public struct literal syntax break as well. Update all construction sites (using call_context: None where appropriate), or provide a compatibility-preserving construction/API strategy before merging.
[RULE] breaking-public-struct-change ·
| _ => Some("stop".to_string()), | ||
| }; | ||
| ModelResponse { | ||
| output: Vec::new(), |
There was a problem hiding this comment.
Populate normalized Responses output and execution metadata
The OpenAI Responses endpoint is not a legacy provider, yet this parser unconditionally reports an empty ordered output and no execution metadata. Downstream callers therefore lose the provider's ordered reasoning, text, tool-call, status, identity, and detailed usage information despite ModelResponse explicitly exposing those normalized fields. Map the parsed Responses items and response metadata into ModelOutputItem and ModelExecution instead of defaulting them away.
[RULE] preserve-response-metadata ·
| }); | ||
| let mut handle = model.response_handle(&snapshot).unwrap(); | ||
| handle.kind = Some("background".into()); | ||
| deny_network_models(); |
There was a problem hiding this comment.
Restore the network guard after the test
This test changes a process-wide network policy and never restores it. Because integration tests can share the same process and run concurrently, the guard can leak into unrelated tests, causing nondeterministic failures or masking network-dependent behavior. Use a scoped guard or restore the prior state before the test exits.
[RULE] global-state-leak ·
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.0516 · 953,109 in / 59,743 out · 62,840 cached (7%) · gpt-5.6-luna, glm-5.3-flash, deepseek-v4.1-flash
critique: $0.0227 · 373,734 in / 23,986 out · 34,580 cached (9%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0196 · 307,923 in / 23,321 out · 26,852 cached (9%) · gpt-5.6-luna
tests: $0.0000 · 85,561 in / 229 out · 1,408 cached (2%) · deepseek-v4.1-flash
description: $0.0091 · 89,644 in / 9,460 out · 0 cached (0%) · glm-5.3-flash
| /// Returns the default provider spec for a known provider. | ||
| pub fn for_kind(kind: ProviderKind) -> Self { | ||
| match kind { | ||
| ProviderKind::Perplexity => Self::new( |
There was a problem hiding this comment.
Provide a usable default Perplexity model
ProviderSpec::for_kind(ProviderKind::Perplexity) returns an empty model. Callers that construct a provider from the generic ProviderSpec and do not supply an explicit selection will therefore pass an invalid model to the Perplexity adapter, despite this being the advertised default provider configuration. Use a supported Perplexity model as the default, or make the generic provider-construction path require and validate an explicit selection instead of returning an apparently usable spec.
[RULE] invalid-default ·
| { | ||
| return Err(invalid("unsupported forced hosted tool")); | ||
| } | ||
| if !matches!(selection, PerplexitySelection::Preset { .. }) |
There was a problem hiding this comment.
Require forced hosted tools to be declared
When the selection is a preset, this condition skips the check that the forced hosted tool appears in the outgoing tools array. A caller can therefore force any allowlisted hosted tool type without declaring it, producing an inconsistent request and potentially invoking provider-managed capabilities that were not part of the configured tool set. Require the selected hosted tool to be declared regardless of whether the model selection is a preset.
Additional critique observation
Require forced hosted tools to be declared
[RULE] undeclared-tool-choice
A request such as PerplexitySelection::preset("low") with Hosted("web_search") and no options.tools passes this check solely because it uses a preset. The builder cannot know that an arbitrary or future provider preset actually exposes the selected hosted tool, so the request can reach the provider with a forced tool choice that is not present in the declared tool list and be rejected or behave inconsistently. Validate the hosted choice against the declared tools for presets too, or explicitly resolve and validate the preset's tool loadout before accepting it.
Suggested change for this observation (reference only)
if !body
.get("tools")
.and_then(Value::as_array)
.is_some_and(|tools| {
[RULE] undeclared-tool-choice ·
| functions.insert(tool.name.clone(), tool); | ||
| } | ||
| let mut explicit_names = HashSet::new(); | ||
| for tool in &request.tools { |
There was a problem hiding this comment.
Reject explicit tools that shadow system tools
functions is populated from system_tools first, but the duplicate check only tracks names from request.tools. An explicit tool can therefore reuse a reconstructed system tool's name and overwrite its schema in the outgoing request, changing the effective tool set and potentially routing a call to the wrong implementation. Reject names already present in functions before inserting explicit tools.
[RULE] tool-name-collision ·
1 participant
Summary
Add a native Perplexity Agent API provider so callers can use one explicit key with a provider-qualified model, fallback models, or a preset. Preserve rich output and deliver real incremental stream events, with explicit background recovery and generated-file access.
Related issue
None.
API or behavior changes
PerplexityModel, typed configuration/options, andProviderKind::Perplexity. Use/v1/agent, with an explicit/v1/responsescreation alias.Provider limitation observed live
Direct HTTP probes independently reproduced HTTP 400 when
previous_response_idreferences a response ending in a custom function call, for both Google and OpenAI. Even plain user follow-ups to those parents failed; completed assistant-text parents worked. Explicit full call/result replay succeeded. The adapter preserves that error without silently resubmitting or changing strategies. The exact internal provider cause is unconfirmed.Cancellation was acknowledged as
cancelling, followed byincomplete; the adapter preserves the reported status rather than treating acknowledgement as confirmed cancellation.Validation
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo build --all-targets --all-featurescargo test --all-features— 1,082 passing unit, integration, and documentation testsRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-featuresgit diff --cached --checkLive verification covered Google
/agent, OpenAI/responses, preset web search with source/citation-marker preservation, incremental text, signed function calls/replay, ordinary conversation continuation, background reconnect/retrieval, cancellation acknowledgement, and CSV generation/listing/download.cargo run -p tinyinference-llm --example perplexity_function_replaycompleted successfully with explicitly supplied credentials.Rust 1.88 MSRV, default-feature-only tests, cargo-deny, and coverage measurement were not run locally. The corresponding extra toolchain/tools are not installed locally.
Tests
Offline fixtures cover request validation, known and unknown output, exact decimal cost rounding, split UTF-8/SSE frames, provisional null content/empty model fields observed live, function argument/signature preservation, authoritative snapshots, partial failures, size/deadline bounds, retry classification, credential redaction, scoped recovery handles, file access, and the public network guard.
Remote MCP/connector integrations and every model/hosted-tool combination have not been tested live.
Documentation
docs/migrations/perplexity-agent.md: contracts, migration changes, retries, storage, and live limitations.crates/tinyinference-llm/examples/perplexity_function_replay.rs: explicit signed call/result replay.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the descriptionReview follow-up
Reviewed all ten inline threads (including the two duplicated lane findings) and both additional concerns in the test-lane summary against the full source. The automated reviewer reported unavailable code retrieval; several findings were based on incomplete context.
apply_toresponses_aliascontrols creation only, with canonical Agent recovery routes. Expanded the submit/poll/cancel test to exercise alias creation followed by/agent/{id}recovery.OpenAiModel::from_specrejects Perplexity. Clarified theProviderSpec.modelrustdoc. A Sonar default would select a different API contract.Transport::send, which checks the guard. The isolated public integration test exercises invoke, stream, submit, retrieve, resume, cancel, list, download, and deferred polling.tests/perplexity_api.rsis a separate Cargo integration-test binary containing one test. Other integration files and unit tests execute in different processes. No concurrent unrelated case shares this process.timeout_at(deadline, work)wraps the entire send/retry future. Added a paused-clock regression proving a pending transport send is cancelled at the request deadline.starting_after, with a complete authoritative terminal response. The existing reconnect regression verifies old events are skipped, new deltas survive, and the final answer is complete.All five required local checks passed again after these changes (1,082 tests). No live paid calls were needed for this follow-up. Review threads have not been manually marked resolved.