Repository navigation
Remove duplicated code found by tinyanalyzer - #312
Conversation
Update the vendored tinyinference submodule to the latest commit. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Fixed several issues across the harness: dialect detection now handles edge cases correctly, cache hashing produces stable keys, structured output validation rejects malformed input, and tool prompts render as intended. These corrections prevent subtle mismatches that could cause incorrect agent behaviour. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Delete the SQLite row decoding helpers and the agent-team coordination operations, which are no longer referenced now that the run ledger handles these paths elsewhere. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reordered the tinyinference_llm imports in dialect.rs and dropped an unused ContentBlock import, and removed a stray blank line at the end of cache/hash.rs. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reworked the run ledger ops to reduce duplication and make the operation flow easier to follow. Behaviour is unchanged. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reworked the run ledger ops to reduce duplication and make the operation flow easier to follow. Behaviour is unchanged. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The paged list functions for agent runs, workflow runs, and agent teams each rebuilt the same filter, count, and limit/offset SQL by hand. A new ListQuery helper now builds that query once, so the three call sites only declare their filters and columns. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Checkpoints can now be persisted to disk and to SQLite in addition to memory, so graph runs survive process restarts. The boundary module was updated to route checkpoint reads and writes through the new backends. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformatted two `let parent_config` bindings that had been split across lines so they fit on one line, matching the surrounding style. No behaviour change. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Introduce a testkit module providing shared helpers for harness tests, along with unit tests covering it and the agent loop. This makes it easier to write consistent tests across the harness crate. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformat the tool_call_response helper so the chained with_usage call stays within the line length limit. No behaviour change. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add tests for the steering module to exercise its behaviour and guard against regressions. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The media test helpers now delegate to the shared testkit response builders instead of constructing ModelResponse values by hand, keeping the fixtures consistent with the rest of the harness. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Remove the unused AssistantMessage and ContentBlock imports from the media tests, keeping only Message which is still referenced. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adjust the integration tests and example to match the updated agent loop and tool deferral interfaces. The changes keep the end-to-end coverage aligned with current behaviour. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Wrap the tool_call_response helper chains across lines in the integration test suite so they satisfy the formatter's line width limit. No behaviour changes. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Introduce a paging module that splits session history into pages so long conversations can be loaded incrementally instead of all at once. Tests cover the new paging behaviour. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The run-ledger list functions now build their queries with the shared `PagedQuery` helper instead of a local `ListQuery` struct, which removes the duplicated paging and filtering logic. The `paging` module is made public so the run-ledger ops can reach it. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Paging parameters are now set on the query via a page method instead of being passed to fetch, so callers configure the page alongside the other filters. This keeps the fetch signature focused on the query itself and makes the paging bounds reusable across calls. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Session listing and search now build their filters, count, and pagination through the shared PagedQuery helper instead of hand-rolled SQL assembly. This removes duplicated parameter-indexing logic while keeping the same filtering and ordering behaviour. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Warning Review limit reached
This review includes 29 billable files and costs up to $7.25. Or wait 5 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (29)
Comment |
Update the vendored tinyinference submodule to a newer revision. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3460271ae
ℹ️ 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>
Tiny Sweeper reviewTiny Sweeper completed its review; deterministic results follow. State: Changes requested Review snapshot
Completeness: Complete What changedNo supported behavioral explanation was produced. Features
Tests
Findings
Resolved this pass
Before merge
How this fits togetherflowchart LR
n0["invoke_default"]:::impacted
n1["invoke_in_context"]:::impacted
n2["..._node_drives_harness_agent_with_tool_loop"]:::impacted
n3["CaptureMiddleware"]:::impacted
n4["...i_step_tool_loop_calls_tool_then_finishes"]:::impacted
n5["...rn_response_completes_with_one_model_call"]:::impacted
n2 -->|calls| n1
n2 -->|tests| n1
n4 -->|calls| n0
n4 -->|tests| n0
n4 -->|uses| n3
n5 -->|calls| n0
n5 -->|tests| n0
n5 -->|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
|
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.0480 · 1,076,373 in / 40,775 out · 91,632 cached (9%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0225 · 425,259 in / 18,009 out · 47,499 cached (11%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0238 · 449,752 in / 15,871 out · 37,669 cached (8%) · gpt-5.6-luna
tests: $0.0010 · 120,184 in / 3,624 out · 4,928 cached (4%) · glm-5.3-flash
description: $0.0003 · 38,795 in / 299 out · 1,408 cached (4%) · glm-5.3-flash
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
The previously-blocking findings are resolved. Clearing the changes request.
$0.0053 · 213,138 in / 8,621 out · 14,578 cached (7%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0023 · 52,559 in / 3,367 out · 8,968 cached (17%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0019 · 39,102 in / 1,114 out · 5,610 cached (14%) · gpt-5.6-luna
tests: $0.0003 · 39,270 in / 291 out · 0 cached (0%) · glm-5.3-flash
description: $0.0003 · 39,373 in / 246 out · 0 cached (0%) · glm-5.3-flash
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0067 · 491,174 in / 20,079 out · 43,857 cached (9%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0025 · 159,186 in / 6,921 out · 20,847 cached (13%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0035 · 208,786 in / 9,769 out · 21,474 cached (10%) · gpt-5.6-luna
tests: $0.0001 · 39,441 in / 539 out · 0 cached (0%) · glm-5.3-flash
description: $0.0003 · 39,596 in / 410 out · 1,408 cached (4%) · glm-5.3-flash
| let (sessions, total) = crate::paging::PagedQuery::default() | ||
| .eq("status", status.map(str::to_owned)) | ||
| .eq("parent_session_id", parent_id.map(str::to_owned)) | ||
| .page(lim, off)? |
There was a problem hiding this comment.
Keep pagination parameters out of reusable query state
page stores the request's limit and offset inside the reusable PagedQuery builder. Any later fetch through the same builder inherits that pagination, making query reuse prone to silently returning the wrong page. Keep pagination as fetch-time arguments (or otherwise make it per-fetch state) rather than storing it on the reusable filter builder.
[RULE] stateful-pagination ·
| .eq_nonblank("kind", request.kind.as_deref()) | ||
| .eq_nonblank("parent_run_id", request.parent_run_id.as_deref()) | ||
| .eq_nonblank("parent_thread_id", request.parent_thread_id.as_deref()) | ||
| .page(limit, offset)? |
There was a problem hiding this comment.
Keep pagination parameters out of reusable query state
PagedQuery still stores limit and offset as builder fields, and these list operations pass pagination into that reusable query state. This couples filtering and pagination and allows a query builder reused or extended for another fetch to carry stale paging parameters. Keep pagination as fetch-time arguments rather than fields on the reusable filter builder, so only the final fetch applies the page.
Additional critique observation
Keep pagination parameters out of reusable query state
[RULE] pagination-state-isolation
The shared PagedQuery is a reusable query builder, but this call stores limit and offset in the same builder state as the filters before fetch executes. If a builder is reused or extended for another page, the pagination values remain attached and can silently affect the later query. Keep page arguments local to fetch, or otherwise ensure pagination state is consumed and cleared when fetching.
[RULE] pagination-state ·
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.0593 · 1,166,361 in / 58,582 out · 112,284 cached (10%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0288 · 509,177 in / 29,440 out · 53,110 cached (10%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0293 · 534,057 in / 25,436 out · 59,174 cached (11%) · gpt-5.6-luna
tests: $0.0004 · 39,431 in / 126 out · 0 cached (0%) · glm-5.3-flash
description: $0.0004 · 39,534 in / 188 out · 0 cached (0%) · glm-5.3-flash
| pub mod entry_tree; | ||
| mod migrations; | ||
| pub mod ops; | ||
| mod paging; |
There was a problem hiding this comment.
Add the paging module implementation
This declaration requires crates/tinyagents-session/src/paging.rs (or an equivalent module declaration), but that file is absent at this commit. Existing callers already reference crate::paging::PagedQuery, so the crate fails to compile with a missing module error.
[RULE] missing-module ·
| })? as usize; | ||
|
|
||
| let limit = request.limit.unwrap_or(50).min(500) as i64; | ||
| let offset = request.offset.unwrap_or(0) as i64; |
There was a problem hiding this comment.
Reject overflowing agent-run pagination offsets
A u64 offset greater than i64::MAX wraps to a negative i64 here. SQLite then receives an invalid pagination offset, so an otherwise valid request can return an error or incorrect results. Use the same checked conversion already applied to workflow runs and teams.
[RULE] integer-overflow ·
Summary
Removes duplicated code found by tinyanalyzer's new clone detection (tinyhumansai/tinyanalyzer#8). The net change is about 1,600 lines removed: +463 / −2,085 across 29 files.
replace_text_blocks(two of them),canonical_value, and the JSON-schema validator (validate_at/validate_type/matches_type). It now calls the versions tinyinference exports, so structured output and tool-call arguments are validated by one implementation.validate_valuekeeps its signature and error type.run_ledger/ops/team.rsandops/rows.rs(1,092 lines) were never declared by anymod, so they were never compiled. Every function in them also exists inops.rs; the few that differ are the older versions.get_logged/get_row_by_idpair replaces six copies of the "open connection, init schema, look up by id, log" code.paging::PagedQueryreplaces five copies of the "optional filters, count, LIMIT/OFFSET, map" code:list_agent_runs,list_workflow_runs,list_agent_teams,list_sessionsandsearch_sessions_inner. Placeholder numbering now lives in one place.Checkpoint::config()andCheckpoint::parent_config()replace 12 hand-builtCheckpointConfigcopies across the file, sqlite and default checkpointers andcompiled/boundary.rs.testkit::text_responseandtestkit::tool_call_responsereplace hand-writtenModelResponsebuilders in the harness unit tests (agent_loop, steering, media) and in 7 integration-test files.Measured with tinyanalyzer on this branch, production duplication goes from 10,319 to 8,609 lines (440 groups, down from 458).
Depends on
tinyhumansai/tinyinference#64, which made the three helpers public, is merged.
vendor/tinyinferencepoints at mergedmain(a32eec8). That commit also brings in tinyinference#60 (sanitize changes); the full suite passes with it.API or behavior changes
Checkpoint::config,Checkpoint::parent_config,testkit::text_response,testkit::tool_call_response.type, now reads… must be an object with the declared fields, got <kind>. That is the harness's existing wording, now produced by tinyinference.Validation
Run on this branch after merging upstream
main(2cc3982):cargo fmt --checkpassescargo clippy --workspace --all-targets --all-features -- -D warningspassescargo build --workspace --all-targetspassescargo test --workspace --all-featurespassesSome runs on this machine had flaky failures in
workspace::git,providers::claude_agent_sdkand onetinyagents-runtimeprefix-refresh test. All of them write to/tmp, which ran out of disk quota during the run. Each passes on rerun, and none touch code changed here.Tests
paging/mod_tests.rs: filters, total count versus page, offset numbering after the filters, blank and absent filters, custom clauses.Not done, and why
crates/tinyagents-harness/src/tool/injected.rs,injected_test.rsandtool/test.rs(777 lines) are also never compiled. They aren't stale copies:injected.rsis an injected-tool-argument feature that was never wired in. Whether to wire it in or delete it is a product call, so it's left alone here.parse_selectionis copied in three integration-test and example files. Examples and tests can't share a module without#[path]tricks, so it's left as is.ModelResponseliterals across the test suites could move to the new testkit builders, but that's a mechanical follow-up.