feat(inspector): add model call statistics - #7277
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
🚅 Deployed to the ironclaw-pr-7277 environment in ironclaw-ci-preview
|
|
@claude review |
This comment was marked as resolved.
This comment was marked as resolved.
|
@ironloopai review |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. ⬛ Final result · Stopped
Manual command by think-in-universe · attempt 1 of 3 · stopped after 13m 32s IronLoop stopped because the pull request target branch or head changed while this Run was active. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/loop/ironclaw_loop_host/src/lib.rs (1)
2101-2125: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMark dropped model-call captures as partial.
A full queue drops
ModelCallcaptures with only a debug log. The Inspector sink receives no loss signal. Saturation can therefore show lower call, token, and latency totals as complete statistics.Retain the non-blocking queue. Add a run-scoped loss marker or truncation state that reaches the Inspector snapshot. Add a caller-level saturation test that verifies the Stats tab reports partial diagnostics.
Based on PR objectives, partial and truncated diagnostic data must be explicit. As per path instructions, “Fail loud” prohibits silently continuing with poisoned state.
🤖 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 `@crates/loop/ironclaw_loop_host/src/lib.rs` around lines 2101 - 2125, Update BufferedPromptDiagnosticSink::enqueue to propagate a run-scoped loss or truncation marker when a ModelCall capture is dropped because the queue is full, while preserving the non-blocking try_send behavior. Ensure the marker reaches the Inspector snapshot so model-call statistics are reported as partial rather than complete, and add a caller-level saturation test verifying the Stats tab exposes partial diagnostics.Source: Path instructions
🤖 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.
Inline comments:
In `@crates/loop/ironclaw_loop_host/src/lib.rs`:
- Around line 1578-1588: Remove the final model_profile_id fallback from
effective_model construction in the host loop, preserving None when
diagnostic_effective_model and diagnostic_effective_model evidence are
unavailable. Propagate the optional value through capture and the Inspector
persistence/display contract so missing provider evidence is represented as
unavailable rather than as a logical profile. Add a caller-level test covering
absent evidence with a nonzero fallback index.
In `@crates/product/ironclaw_assistant/src/inspector_store.rs`:
- Around line 385-402: In
crates/product/ironclaw_assistant/src/inspector_store.rs:385-402, update the
model-call statistics path to use a bounded identity set of counted call_id
values independent of the retained model_calls deque, preventing evicted Started
records from being recounted on terminal updates. Apply the same approach at
crates/product/ironclaw_assistant/src/inspector_store.rs:413-435 for activity_id
and tool-call statistics. Add a regression test that fills
max_model_calls_per_run, evicts a Started record, records its Succeeded update,
and verifies total_model_calls is unchanged.
- Around line 632-651: Update update_model_count so calls_per_model_truncated is
cleared when removing an existing model causes calls_per_model to fall below
MAX_MODELS_IN_STATS, allowing the breakdown to be reported as complete again. If
the flag is intentionally monotonic because omitted models cannot be recovered,
preserve the latch and document that decision directly beside the flag update.
In
`@crates/product/ironclaw_webui/frontend/src/pages/chat/inspector/inspector-panel.tsx`:
- Around line 317-330: Update the calls_per_model row key in the stats rendering
map to use the array index rather than the truncated entry.model.content value.
Correct the partialMetricCount display so it either counts affected calls or
explicitly labels the value as unavailable metric samples across the tracked
metrics; do not present the summed metric count as individual samples when it
represents multiple metrics per call.
In
`@crates/product/ironclaw_webui/frontend/src/pages/chat/inspector/useInspector.ts`:
- Around line 201-206: Update useInspector() so diagnostic updates in the
prompt_updated/model_call/tool_execution_updated/stats branch coalesce snapshot
refreshes with a short trailing debounce or pending-refresh flag instead of
incrementing snapshotGeneration for every event. Also serialize the stream SSE
cursor in the snapshot request’s last-event-id header and stop re-querying it
solely through after_cursor.
In `@tests/e2e/scenarios/test_reborn_webui_v2_smoke.py`:
- Around line 477-490: Move the inline stats selectors used around the inspector
stats assertions into helpers.SEL_V2, adding entries for the stats tab and stats
content and referencing those entries here. Update the “Model calls” and “Input
tokens” assertions to verify the exact rendered values rather than substring
matches, while preserving the existing visibility and other stats checks.
---
Outside diff comments:
In `@crates/loop/ironclaw_loop_host/src/lib.rs`:
- Around line 2101-2125: Update BufferedPromptDiagnosticSink::enqueue to
propagate a run-scoped loss or truncation marker when a ModelCall capture is
dropped because the queue is full, while preserving the non-blocking try_send
behavior. Ensure the marker reaches the Inspector snapshot so model-call
statistics are reported as partial rather than complete, and add a caller-level
saturation test verifying the Stats tab exposes partial diagnostics.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9650ab3-9eee-4aea-80f5-b4d4f6294941
📒 Files selected for processing (21)
crates/app/ironclaw_composition/src/observability/budget.rscrates/contracts/ironclaw_loop_contracts/src/host/model.rscrates/kernel/ironclaw_turns/tests/agent_loop_host_contract.rscrates/loop/ironclaw_agent_loop/src/executor/failure_explanation.rscrates/loop/ironclaw_agent_loop/src/executor/model.rscrates/loop/ironclaw_agent_loop/src/executor/tests/cancellation.rscrates/loop/ironclaw_hooks/src/middleware/model_port.rscrates/loop/ironclaw_loop_host/src/budget_accountant.rscrates/loop/ironclaw_loop_host/src/lib.rscrates/loop/ironclaw_loop_host/src/model_gateway.rscrates/loop/ironclaw_loop_host/tests/llm_gateway.rscrates/loop/ironclaw_loop_host/tests/thread_loop_host_contract.rscrates/loop/ironclaw_turn_runner/src/text_loop_driver.rscrates/product/ironclaw_assistant/src/inspector_store.rscrates/product/ironclaw_assistant/tests/support/planned_agent_loop.rscrates/product/ironclaw_webui/frontend/src/pages/chat/inspector/inspector-panel.test.tsxcrates/product/ironclaw_webui/frontend/src/pages/chat/inspector/inspector-panel.tsxcrates/product/ironclaw_webui/frontend/src/pages/chat/inspector/useInspector.test.tsxcrates/product/ironclaw_webui/frontend/src/pages/chat/inspector/useInspector.tstests/CLAUDE.mdtests/e2e/scenarios/test_reborn_webui_v2_smoke.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/loop/ironclaw_loop_host/src/lib.rs (1)
2310-2310: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftValidate provider model evidence before diagnostic capture.
The response and error fields accept arbitrary
Stringvalues. Line 1513 copies those values intoHostManagedModelCallDiagnosticCapture. A provider can therefore place credential-like or otherwise unsafe text into Inspector diagnostics.Use
ProviderModelIdfor both evidence fields, or validate in both builders and omit invalid evidence. Add a caller-level test that invalid provider evidence never reachesrecord_model_call.
crates/loop/ironclaw_loop_host/src/lib.rs#L2310-L2310: replaceOption<Arc<String>>with validatedProviderModelIdevidence.crates/loop/ironclaw_loop_host/src/lib.rs#L2450-L2450: apply the same validated type to error evidence.crates/loop/ironclaw_loop_host/src/lib.rs#L1513-L1522: preserve only validated evidence when creating the diagnostic capture.As per coding guidelines, “Prefer strong types such as enums and newtypes over strings.” Based on PR objectives, diagnostic captures must contain no credentials or unredacted content.
🤖 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 `@crates/loop/ironclaw_loop_host/src/lib.rs` at line 2310, Replace the response evidence field at crates/loop/ironclaw_loop_host/src/lib.rs:2310-2310 and the error evidence field at crates/loop/ironclaw_loop_host/src/lib.rs:2450-2450 with validated ProviderModelId values instead of Option<Arc<String>>. In the diagnostic capture construction at crates/loop/ironclaw_loop_host/src/lib.rs:1513-1522, propagate only validated evidence so unsafe provider text cannot reach record_model_call. Add a caller-level test verifying invalid provider evidence is omitted and never recorded.Source: Coding guidelines
🤖 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.
Inline comments:
In `@crates/product/ironclaw_assistant/src/inspector_store.rs`:
- Around line 1809-1820: Update
model_breakdown_truncation_remains_latched_after_a_bucket_is_removed to exercise
record_model_call instead of update_model_count: record MAX_MODELS_IN_STATS + 1
distinct effective models, replace one model’s contribution with zero, then
assert snapshot.stats.calls_per_model_truncated remains true. Use the store’s
public record path and its snapshot result so replacement reversal is covered.
In
`@crates/product/ironclaw_webui/frontend/src/pages/chat/inspector/useInspector.ts`:
- Around line 151-157: Update scheduleSnapshotRefresh to retain the first
refresh deadline while continuing to debounce subsequent updates, using
SNAPSHOT_REFRESH_MAX_WAIT_MS as the upper bound. Ensure frequent qualifying
events cannot postpone the snapshot indefinitely, while preserving timer cleanup
and the disposed guard.
---
Outside diff comments:
In `@crates/loop/ironclaw_loop_host/src/lib.rs`:
- Line 2310: Replace the response evidence field at
crates/loop/ironclaw_loop_host/src/lib.rs:2310-2310 and the error evidence field
at crates/loop/ironclaw_loop_host/src/lib.rs:2450-2450 with validated
ProviderModelId values instead of Option<Arc<String>>. In the diagnostic capture
construction at crates/loop/ironclaw_loop_host/src/lib.rs:1513-1522, propagate
only validated evidence so unsafe provider text cannot reach record_model_call.
Add a caller-level test verifying invalid provider evidence is omitted and never
recorded.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33db8bb5-d5b2-435c-b021-7c5cbc3ae74c
📒 Files selected for processing (10)
crates/loop/ironclaw_loop_host/src/lib.rscrates/loop/ironclaw_loop_host/tests/thread_loop_host_contract.rscrates/product/ironclaw_assistant/src/inspector_store.rscrates/product/ironclaw_webui/frontend/src/pages/chat/inspector/inspector-panel.test.tsxcrates/product/ironclaw_webui/frontend/src/pages/chat/inspector/inspector-panel.tsxcrates/product/ironclaw_webui/frontend/src/pages/chat/inspector/useInspector.test.tsxcrates/product/ironclaw_webui/frontend/src/pages/chat/inspector/useInspector.tstests/e2e/helpers.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.pytests/support/reborn_parity_qa/binary_e2e.rs
* feat(inspector): add operator inspection API * docs(inspector): assign product service ownership * test(inspector): ratchet diagnostic contracts * feat(inspector): add debug panel shell * test(inspector): cover debug panel shell e2e * fix(inspector): stop diagnostics when panel closes * feat(inspector): add prompt inspection * fix(inspector): follow current webui ownership * feat(inspector): add model call statistics * test(inspector): cover model statistics e2e * fix(inspector): avoid uncollected tool metrics * test(inspector): cover prompt diagnostics e2e * test(inspector): align statistics e2e scope * fix(inspector): redact prompt metadata * fix(inspector): preserve per-call model identity * fix(inspector): classify prompt instruction sources * test(inspector): assert reported token usage * fix(inspector): address review feedback * fix(inspector): retry transient snapshot failures * fix(inspector): address prompt diagnostic review findings * fix(inspector): follow debug query navigation * fix(inspector): preserve stream terminal state * fix(inspector): capture full capability surface * fix(inspector): address prompt diagnostic review feedback * fix inspector model call stats review findings * fix inspector refresh and truncation regressions
Summary
Linked Issue
Closes #7223
Depends on #7222
Part of #7218
Security Impact
Diagnostic statistics remain operator-only, caller-scoped, bounded, and process-local. No credentials or unredacted prompt content are added to the statistics response.
Database Impact
None. No schema or migration changes.
Blast Radius
Limited to model-call diagnostic capture, the operator inspection API, and the Inspector statistics tab.
Rollback Plan
Revert this PR to remove model-call aggregation and the Inspector statistics tab without changing the underlying diagnostic session storage.