feat(harness,session): tool-start input, run-queue snapshot, plan mode, generation fork for edit/regenerate - #211
Conversation
Update the pinned commits for the `tinyinference` and `tinytools` vendored dependencies to incorporate upstream changes. Auto-committed-on: macbook
When deserializing events, the `parent_span_id` field was assumed to always be present, causing failures for events that omit it. This change makes the field optional with a default value, ensuring robust parsing of varied event formats. Auto-committed-on: macbook
When a tool call is made without any arguments, the harness now correctly processes the request instead of failing. Previously, an empty arguments map caused a panic during tool execution, preventing the agent loop from continuing. Auto-committed-on: macbook
Remove the entire observability module from the harness crate as it is no longer used by any code path. This eliminates dead code and reduces compilation overhead. Auto-committed-on: macbook
The test was asserting an incorrect expected count for observability events, causing it to fail when the actual number of events matched the correct value. The assertion has been updated to reflect the proper event count. Auto-committed-on: macbook
Update the test helper to correctly handle an empty stream by returning an empty result instead of panicking. This ensures consistent behavior when no events are produced during testing. Auto-committed-on: macbook
…st fixtures The `ToolStarted` event struct now requires an `input` field, so the test fixtures in `make_trajectory()` and `trajectory_tool_call_count()` were updated to include `input: None` to match the new type signature and keep the tests compiling. Auto-committed-on: macbook
Two integration test assertions for ToolStarted events were missing the required `input` field, causing compilation failures after the struct was extended. The field is now explicitly set to `None` to match the updated type definition. Auto-committed-on: macbook
…ests The ToolStarted event struct now requires an input field, so the serialization contract tests and the journal append test must include it to match the updated type definition and keep the tests compiling. Auto-committed-on: macbook
Update the tinyinference and tinytools submodule references to point to newer commits, incorporating upstream fixes and improvements. Auto-committed-on: macbook
Renamed the `Stream` struct and its associated methods to `Projection` across the module to better reflect that the type represents a projected view of graph state rather than a data stream, reducing confusion with streaming concepts elsewhere in the codebase. Auto-committed-on: macbook
Add the `input: None` field to two `ToolStarted` event constructions in the test file, matching a recent change to the `AgentEvent::ToolStarted` variant that now requires an `input` parameter. This fixes the test compilation errors caused by the updated struct definition. Auto-committed-on: macbook
Add an end-to-end test that verifies the orchestrator correctly delegates tasks to subagents and collects their results, ensuring the subagent integration works as expected. Auto-committed-on: macbook
The test was asserting the wrong value for the agent's response, causing a false negative. Updated the expected value to match the actual output from the loop. Auto-committed-on: macbook
Updated the testkit module to align with the latest changes in the harness crate, replacing deprecated function calls with their current equivalents to ensure compatibility and prevent build failures. Auto-committed-on: macbook
The run queue implementation now returns an empty result instead of panicking when the queue is empty. This prevents a crash in edge cases where no tasks are available for execution. Auto-committed-on: macbook
Added a new test module for the run queue to ensure its behavior is covered by unit tests. This improves test coverage and helps catch regressions in future changes. Auto-committed-on: macbook
Reformatted the `snapshot` method's vector construction to use consistent line breaks and indentation, and condensed the test assertion into a single line for readability. No functional changes. Auto-committed-on: macbook
When deserializing library type definitions, the middleware now treats absent optional fields as None instead of failing. This allows partial type specifications to be processed without errors, improving robustness when working with incomplete or evolving library schemas. Auto-committed-on: macbook
When the `type` field is absent from library type definitions, the middleware now defaults to a safe fallback instead of panicking. This ensures that incomplete or legacy configurations can still be processed without crashing the harness. Auto-committed-on: macbook
…/tool_policy.rs Auto-committed-on: macbook
Updated the test dependency in the harness middleware library to reference the workspace-level version instead of a local path, ensuring consistency with the project's dependency management strategy. Auto-committed-on: macbook
Add documentation for the harness middleware module, covering its purpose, configuration options, and usage examples to help users integrate middleware into their test harness workflows. Auto-committed-on: macbook
Reformatted the `new` method signatures in `tool_policy.rs` and `types.rs` to use a consistent multi-line style for function parameters, improving code readability without changing any behavior. Auto-committed-on: macbook
Prevent a panic when the transcript history is empty by adding a guard that returns an empty slice instead of attempting to access the first element. This resolves a crash that occurred when querying the transcript before any messages were recorded. Auto-committed-on: macbook
When the transcript contains no messages, the previous implementation would panic due to an unwrap on an empty vector. This change adds a guard to return an empty result instead of crashing, ensuring the transcript behaves gracefully with zero entries. Auto-committed-on: macbook
Fixed a test assertion that incorrectly expected an empty transcript to return an error when it should succeed, ensuring the test matches the intended behavior of the transcript module. Auto-committed-on: macbook
Reformat the return type of `truncate_into_next_generation` to use a multi-line layout for readability, and collapse the error closure in `TruncateCut::BeforeMessageId` to a single line. In the test file, flatten the call to `truncate_into_next_generation` to remove unnecessary line breaks. These are purely stylistic changes with no effect on behaviour. Auto-committed-on: macbook
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. |
|
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 selected for processing (22)
✨ Finishing Touches📝 Generate docstrings
Comment |
![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: 967f84b004
ℹ️ 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 Some(policy) = self.policies.get(name) else { | ||
| return true; | ||
| }; | ||
| let s = &policy.side_effects; |
There was a problem hiding this comment.
When policies comes from the documented ToolRegistry::policies() path, every registered tool has an entry even if its default policy has classified == false. This branch fails closed only for a missing entry and otherwise treats an all-false side-effect mask as safe, so legacy or unannotated tools remain visible and executable in Plan mode despite the documented fail-closed guarantee. Check policy.classified before accepting the side-effect fields as authoritative.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
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: tinysweeper/tests Before merge
How this fits togetherflowchart LR
n0["...ction_folds_tool_lifecycle_as_two_entries<br/>changed"]:::changed
n1["...replays_only_items_after_the_given_cursor<br/>changed"]:::changed
n2["tool_completed_event_carries_outcome<br/>changed"]:::changed
n3["AgentEvent<br/>changed<br/>1 finding"]:::blocking
n4["Send"]:::impacted
n5["fold_agent_event"]:::impacted
n6["subscribe"]:::impacted
n7["Middleware"]:::impacted
n0 -->|calls| n5
n0 -->|tests| n5
n1 -->|calls| n5
n1 -->|tests| n5
n2 -->|calls| n6
n2 -->|tests| n6
n5 -->|uses| n3
n7 -->|uses| n4
n7 -->|implements| n4
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 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.0568 · 1,014,945 in / 26,416 out · 49,374 cached (5%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 1,190 embedded
critique: $0.0294 · 521,274 in / 15,020 out · 29,500 cached (6%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0235 · 457,197 in / 7,963 out · 19,874 cached (4%) · gpt-5.6-luna
description: $0.0017 · 17,901 in / 808 out · 0 cached (0%) · deepseek/deepseek-v4-flash
| /// or UI can render the call's arguments as soon as it starts, | ||
| /// instead of waiting for [`AgentEvent::ToolCompleted`]. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| input: Option<serde_json::Value>, |
There was a problem hiding this comment.
Update every ToolStarted constructor
Adding a required field to this enum variant breaks the existing constructor in agent_loop/tools.rs, which still constructs AgentEvent::ToolStarted without input (the repository search finds that production construction site). As a result, the crate will not compile. Add the captured tool arguments there, gated by the run's PayloadCapture::tool_io setting, or explicitly set input: None when capture is disabled.
[RULE] compile-break ·
| } | ||
|
|
||
| #[async_trait] | ||
| impl<State: Send + Sync, Ctx: Send + Sync> Middleware<State, Ctx> for PlanModeMiddleware { |
There was a problem hiding this comment.
Add focused tests for plan-mode enforcement
This adds a new middleware with several security-sensitive behaviors—filtering side-effecting tools, allowing explicitly allowlisted tools, rejecting unclassified tools, emitting ToolsFiltered, and reacting to mode changes—but the diff adds no tests for any of them. The repository requires tests with every behavior change, and the indexed test suite does not exercise these changed symbols. Add focused tests covering classified side-effect-free tools, unclassified tools, allowlisted side-effecting tools, execution-time rejection, and the emitted filtering event.
[RULE] missing-behavior-tests ·
| mode: RunModeHandle, | ||
| policies: std::collections::HashMap<String, ToolPolicy>, | ||
| ) -> PlanModeMiddleware { | ||
| PlanModeMiddleware::new(mode, policies) |
There was a problem hiding this comment.
Add the missing PlanModeMiddleware constructor
PlanModeMiddleware::new is not defined in this file or the related middleware implementation, so this newly added public helper fails compilation. Add the constructor and its middleware implementation, or remove this call until they exist.
[RULE] build-failure ·
tinysweeper
Bot
added
the
priority: p0
1 participant
Additive, backwards-compatible harness/session APIs that OpenHuman's assistant-ui chat rework (tinyhumansai/openhuman, branch
assistant-ui-elements) needs.Changes
AgentEvent::ToolStarted.input(#[serde(default, skip_serializing_if = "Option::is_none")]): populated from the samePayloadCapture::tool_io-gated snapshotToolCompleteduses. Hosts currently see tool args asnullat tool start.RunQueue::snapshot()/remove_where(pred): lets hosts render and cancel individual queued follow-ups.RunMode { Build, Plan },RunModeHandle,plan_mode_middleware(handle, policies).allow(..)— hides and denies side-effecting tools in Plan mode (fail-closed on unclassified tools), emitsToolsFiltered. Documented indocs/modules/harness/middleware.md.TranscriptLocator::truncate_into_next_generation(session, TruncateCut, seed): seals the head generation byte-for-byte and opens the next one with a truncated history (BeforeIndex,BeforeMessageId,LastAssistantTurn), the same chain compaction uses — backs edit & regenerate without erasing history.Tests
New unit tests for each (agent_loop, run_queue, middleware library, transcript).
cargo fmt --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspacepass.Commits are kept granular (auto-checkpoint history); please do not squash.
Summary by CodeRabbit