feat(orchestration): surface tool_result outcome + per-session message_count#4712
Conversation
…e_count OrchestrationMessage now carries ok/is_error/exit_code decoded from the v2 tool_result payload, persisted via additive, migration-guarded messages columns. A failed tool run is therefore distinguishable from a successful one on read instead of both collapsing to plain output. SessionSummary gains message_count (total persisted messages per session) via the existing store::count_messages, for the roster's per-session count. Both unblock the orchestration UI work (failed-result styling + session counts) without touching any other surface.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds optional ChangesTool result outcome and session message count
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
# Conflicts: # src/openhuman/orchestration/store.rs
![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: 33743fafe2
ℹ️ 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".
| b.body = p.output; | ||
| b.call_id = non_empty(p.call_id); | ||
| // Carry the outcome so the renderer can distinguish a failed run. | ||
| b.ok = Some(p.ok); |
There was a problem hiding this comment.
When a v2 tool_result comes from an older or partial harness payload that omits ok, ToolResultPayload accepts it via #[serde(default)] and p.ok becomes false; persisting Some(false) here makes the frontend failure check (ok === false) render that otherwise-successful/unknown result as failed even when is_error is false and exit_code is absent or zero. Only set ok when the wire field was actually present, or derive it from the explicit error/exit fields.
Useful? React with 👍 / 👎.
| let message_count = | ||
| store::count_messages(conn, &session.agent_id, &session.session_id)?; |
There was a problem hiding this comment.
In sessions with v2 session_info, status, lifecycle, or unknown events, this uses count_messages, which counts rows that messages_list, unread_count, and latest_message_preview deliberately filter out. The Connections UI can therefore show a nonzero or steadily growing “N messages” count while the transcript is empty or unchanged due to hidden status traffic; use the same visibility predicate as the transcript query for the summary count.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/openhuman/orchestration/schemas.rs (1)
365-424: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePer-session
count_messagesquery adds another round-trip to the sessions-list loop.Each session now runs
unread_count, an optional preview query, andcount_messages— an N+1 pattern.orchestration_list_sessionsin tools.rs already has the identical shape, so this is consistent with existing precedent rather than a new regression; flagging only as a good-to-have optimization (e.g., a single aggregatedGROUP BY session_idcount query) if session lists grow large.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/orchestration/schemas.rs` around lines 365 - 424, handle_sessions_list currently does a per-session count_messages lookup inside the loop, creating an N+1 round-trip pattern alongside unread_count and the optional preview query. Refactor the sessions_list path to fetch message counts in one aggregated query up front (for example by session_id) and then reuse that result when building each SessionSummary, keeping the existing summarize/current_task logic intact. Use handle_sessions_list as the main entry point and align the shape with orchestration_list_sessions in tools.rs so the optimization stays consistent with the existing session-list flow.src/openhuman/orchestration/store.rs (1)
45-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment doesn't mention the new outcome columns.
The comment above the
messagestable (lines 45-47) still only documentsevent_kind/tool_name/call_id; consider extending it to mentionok/is_error/exit_codefor consistency with the rest of the file's documentation style.📝 Suggested doc update
- // `event_kind`/`tool_name`/`call_id` carry the v2 per-message event shape - // (`event.kind` + tool identity/correlation). Nullable and additive; v1 and - // pinned master/subconscious rows leave them NULL. + // `event_kind`/`tool_name`/`call_id` carry the v2 per-message event shape + // (`event.kind` + tool identity/correlation). `ok`/`is_error`/`exit_code` + // carry the `tool_result` outcome. Nullable and additive; v1 and pinned + // master/subconscious rows leave them NULL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/orchestration/store.rs` around lines 45 - 63, The `messages` table doc comment in `store.rs` is outdated because it only describes `event_kind`/`tool_name`/`call_id` and omits the new outcome columns. Update the comment above `CREATE TABLE IF NOT EXISTS messages` to also document `ok`, `is_error`, and `exit_code`, keeping the wording consistent with the existing schema comments in `messages`/`store.rs`. Use the table definition as the source of truth and make sure the new fields are mentioned alongside the other nullable additive v2 columns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/orchestration/schemas.rs`:
- Around line 365-424: handle_sessions_list currently does a per-session
count_messages lookup inside the loop, creating an N+1 round-trip pattern
alongside unread_count and the optional preview query. Refactor the
sessions_list path to fetch message counts in one aggregated query up front (for
example by session_id) and then reuse that result when building each
SessionSummary, keeping the existing summarize/current_task logic intact. Use
handle_sessions_list as the main entry point and align the shape with
orchestration_list_sessions in tools.rs so the optimization stays consistent
with the existing session-list flow.
In `@src/openhuman/orchestration/store.rs`:
- Around line 45-63: The `messages` table doc comment in `store.rs` is outdated
because it only describes `event_kind`/`tool_name`/`call_id` and omits the new
outcome columns. Update the comment above `CREATE TABLE IF NOT EXISTS messages`
to also document `ok`, `is_error`, and `exit_code`, keeping the wording
consistent with the existing schema comments in `messages`/`store.rs`. Use the
table definition as the source of truth and make sure the new fields are
mentioned alongside the other nullable additive v2 columns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 122ac26d-e85e-4b8c-9cc9-0f926b9eaff1
📒 Files selected for processing (4)
src/openhuman/orchestration/ingest.rssrc/openhuman/orchestration/schemas.rssrc/openhuman/orchestration/store.rssrc/openhuman/orchestration/types.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 592341d427
ℹ️ 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 message_count = | ||
| store::count_messages(conn, &session.agent_id, &session.session_id)?; |
There was a problem hiding this comment.
When the existing master window contains rows for more than one agent_id (for example local Master asks plus peer plain DMs/outbound steering), the transcript reader aggregates that pinned chat by session_id only via store::list_messages_by_session, while this new message_count is scoped to the current session row's agent_id. In that context the roster can report a messageCount smaller than the number of messages shown when opening the Master thread; use the same session-only scope for pinned windows or keep their count at the placeholder value.
Useful? React with 👍 / 👎.
2 participants
Summary
OrchestrationMessagenow carriesok/is_error/exit_code, decoded from the v2 harnesstool_resultpayload, so a failed tool run is distinguishable from a successful one instead of both collapsing to plainbodytext.messagescolumns (add_column_if_missing), matching the existingevent_kind/tool_name/call_idpattern — existing rows read back asNULL→None.SessionSummarygainsmessage_count(total persisted messages per session), populated via the existingstore::count_messages.Problem
The orchestration message stream flattened the v2
ToolResultPayloadtobodyonly, droppingok/is_error/exit_code. Any renderer therefore could not tell a failed tool result from a successful one — they render identically. The roster also had no per-session total message count (onlyunread).Solution
ingest.rs: captureok/is_error/exit_codein theToolResultbranch, thread them throughClassifiedMessageand the persistedOrchestrationMessage.store.rs: add the three nullableINTEGERcolumns to the schema DDL +migrate()(idempotent existence-checked ALTERs), and toinsert_message/map_message_row/ the three messageSELECTlists.schemas.rs: addmessage_countto theSessionSummaryDTO, computed per session inhandle_sessions_listviastore::count_messages.types.rs: add the fields (camelCase serde, skip-if-none).Submission Checklist
summarizemessage_count.orchestration::{ingest,store,schemas}).docs/TEST-COVERAGE-MATRIX.md.Impact
add_column_if_missing; a fresh DB gets them from the DDL, an existing store gains them with rows defaultingNULL. No backfill, no breaking change. Serialized fields areskip_serializing_if = Option::is_none, so existing clients are unaffected.Related
ok/isError/exitCode+messageCount(orchestration Agent chat + Connections redesign).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check— Rust-only change.pnpm typecheck— Rust-only change.cargo test --lib orchestration::{ingest,store,schemas}(46 passed); fullorchestration::suite 387 passed.cargo fmtapplied; crate compiles.app/src-taurinot touched.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
tool_resultoutcome + per-session count now available on the wire.Parity Contract
None.derive_status/readers unchanged; NULL columns read asNone.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes