[split 1/3] Reborn hardening: error_summary sanitization, chaos guards, provider-name boundary - #5962
ilblackdragon wants to merge 1 commit into
Conversation
…ng, provider-name boundary, misc fixes Extracted from #5279 — robustness/hardening/test-hygiene not tied to the queued-message or budget features: - error_summary/safe_summary sanitization surfaced end-to-end (events, projections, streams, product adapter boundary re-validation) - agent-loop chaos handling: model_gateway bounds guards, milestone events - external-tool provider-name boundary (operator_env) - nearai-mcp hermetic env tests, heartbeat-storage tolerance, webui graceful-shutdown drain, Cargo.toml-as-document parse fix, stack-overflow-in-outbound-delivery test fix
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
scope: extensions
github-actions
Bot
added
risk: medium
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds sanitized optional capability error summaries across runtime events, milestones, projections, live updates, and product views. It also bounds WebUI graceful-shutdown draining and hardens capability-token range handling and related tests. ChangesCapability error summary pipeline
WebUI graceful shutdown
Safety and maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces error_summary across event projections, streams, and views, adding sanitization and validation to prevent sensitive leaks. It also hardens string slicing in model_gateway.rs, implements a graceful shutdown timeout for the WebUI ingress server, and refactors configuration tests to avoid environment variable locks. Feedback focuses on restoring an accidentally deleted line in a doc comment in manager.rs and reverting several test assertions from assert!(a == b) back to assert_eq! to ensure better debuggability on failure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| /// `Ok(None)` if the manifest is valid but lacks a package name, allowing | ||
| /// callers to fall back to the extension name in that case. |
There was a problem hiding this comment.
The first line of the doc comment describing the function's purpose was accidentally removed. Restoring it makes the documentation complete and grammatically correct.
/// Read [package].name from the Cargo.toml at source_dir. Returns
/// Ok(None) if the manifest is valid but lacks a package name, allowing
/// callers to fall back to the extension name in that case.
| let out = truncate_env_value_for_display("12"); | ||
| assert!(out == "12"); // safety: truncation contract assertion in test module. |
There was a problem hiding this comment.
Using assert_eq! is preferred over assert!(a == b) in tests because assert_eq! automatically prints both the left and right values on failure, making test failures much easier to debug.
| let out = truncate_env_value_for_display("12"); | |
| assert!(out == "12"); // safety: truncation contract assertion in test module. | |
| assert_eq!(truncate_env_value_for_display("12"), "12"); |
| let char_count = out.chars().count(); | ||
| assert!(char_count == 65); // safety: truncation contract assertion in test module. |
There was a problem hiding this comment.
Using assert_eq! is preferred over assert!(a == b) in tests because assert_eq! automatically prints both the left and right values on failure, making test failures much easier to debug.
| let char_count = out.chars().count(); | |
| assert!(char_count == 65); // safety: truncation contract assertion in test module. | |
| assert_eq!(out.chars().count(), 65); |
| if pending_count != 0 { | ||
| panic!("default-on auto-approve must not create a pending approval"); | ||
| } | ||
| assert!(pending_count == 0); // safety: test-only assertion in #[cfg(test)] module. |
There was a problem hiding this comment.
Using assert_eq! with a descriptive message is preferred over assert!(a == b) because it preserves the original panic context while using standard assertions.
| assert!(pending_count == 0); // safety: test-only assertion in #[cfg(test)] module. | |
| assert_eq!(pending_count, 0, "default-on auto-approve must not create a pending approval"); |
| let capability_id = tool_definition.capability_id.as_str(); | ||
| assert!(capability_id == "external_tool.client_lookup"); // safety: provider-name contract assertion. |
There was a problem hiding this comment.
Using assert_eq! is preferred over assert!(a == b) in tests because assert_eq! automatically prints both the left and right values on failure, making test failures much easier to debug.
| let capability_id = tool_definition.capability_id.as_str(); | |
| assert!(capability_id == "external_tool.client_lookup"); // safety: provider-name contract assertion. | |
| assert_eq!(tool_definition.capability_id.as_str(), "external_tool.client_lookup"); |
| let capability_id = candidate.capability_id.as_str(); | ||
| assert!(capability_id == "external_tool.client_lookup"); // safety: provider-call staging assertion. |
There was a problem hiding this comment.
Using assert_eq! is preferred over assert!(a == b) in tests because assert_eq! automatically prints both the left and right values on failure, making test failures much easier to debug.
| let capability_id = candidate.capability_id.as_str(); | |
| assert!(capability_id == "external_tool.client_lookup"); // safety: provider-call staging assertion. | |
| assert_eq!(candidate.capability_id.as_str(), "external_tool.client_lookup"); |
There was a problem hiding this comment.
Pull request overview
This PR is the first slice of a stacked Reborn-hardening series, extending the runtime/tool activity event model to carry a sanitized error_summary end-to-end, adding defensive parsing/loop guards, and tightening a few boundary/contract behaviors (WebUI shutdown drain bounding, provider-name contract, and hermetic env-based config tests).
Changes:
- Add
error_summary/safe_summaryplumbing across runtime events, projections, event streams, WebUI views, and preview rendering, with re-validation at the product-adapter boundary. - Harden agent-loop parsing in
model_gateway(char boundary / bounds guards) and expand milestone failure events to optionally includeLoopSafeSummary. - Improve operational/test robustness: bounded graceful-shutdown drain for WebUI ingress, hermetic NearAI MCP env config tests, and a few local-dev/runtime contract tweaks.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/extensions/manager.rs | Adjusts Cargo.toml package-name parsing documentation around extension crate-name reading. |
| crates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rs | Updates schema contract fixtures to include error_summary. |
| crates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rs | Updates handler contract fixtures to include error_summary. |
| crates/ironclaw_turns/src/run_profile/milestones.rs | Extends milestone failure kind with optional safe_summary and defaulting. |
| crates/ironclaw_triggers/tests/repository_contract.rs | Adds “safety” clarifiers around raw SQL test setup. |
| crates/ironclaw_reborn/tests/loop_milestone_event_projection.rs | Updates milestone projection tests for new safe_summary field. |
| crates/ironclaw_reborn/src/model_gateway.rs | Adds bounds/char-boundary guards when slicing capability-request tokens. |
| crates/ironclaw_reborn/src/milestone_events.rs | Projects milestone safe_summary into runtime events’ error_summary + adds tests. |
| crates/ironclaw_reborn_webui_ingress/tests/serve_loop.rs | Adds a contract test ensuring graceful shutdown drain is time-bounded. |
| crates/ironclaw_reborn_webui_ingress/src/lib.rs | Implements bounded graceful-shutdown drain timeout for the WebUI server loop. |
| crates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rs | Adjusts tokio test flavor to multi-thread for outbound-delivery selection test. |
| crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs | Tightens external-tool provider-name contract assertions in local-dev runtime tests. |
| crates/ironclaw_reborn_composition/src/projection/tests/runtime_stream.rs | Updates projection tests to include error_summary in activity payloads. |
| crates/ironclaw_reborn_composition/src/projection/tests/live_progress_stream.rs | Adds LoopSafeSummary usage + asserts error_summary is surfaced in live stream. |
| crates/ironclaw_reborn_composition/src/projection/tests/display_preview.rs | Expands preview tests to ensure failed activities prefer error_summary and preserve visible fields parity. |
| crates/ironclaw_reborn_composition/src/projection/tests/display_preview_runtime.rs | Updates preview fixture builders to include error_summary. |
| crates/ironclaw_reborn_composition/src/projection/live_progress.rs | Threads safe_summary/error_summary into live-progress milestone projection. |
| crates/ironclaw_reborn_composition/src/projection/display_preview.rs | Uses error_summary for failed previews and preserves running subtitle/input_summary when available. |
| crates/ironclaw_reborn_composition/src/projection.rs | Adds error_summary to outbound runtime payload mapping. |
| crates/ironclaw_reborn_composition/src/nearai_mcp.rs | Refactors env bootstrap config parsing for hermetic tests (value-based helper). |
| crates/ironclaw_reborn_composition/src/factory/local_dev_host_tests/approval_gates.rs | Minor test refactors/comments and an assertion style change in approval-gate tests. |
| crates/ironclaw_reborn_cli/src/operator_env.rs | Tweaks env-value truncation tests and assertion style. |
| crates/ironclaw_product_adapters/src/outbound.rs | Adds error_summary to CapabilityActivityView with strict boundary validation + new tests. |
| crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs | Updates milestone pattern matches to tolerate added fields (..). |
| crates/ironclaw_loop_support/src/capability_port.rs | Populates milestone safe_summary from host/runtime failure summaries when valid. |
| crates/ironclaw_host_runtime/src/first_party_tools/shell.rs | Preserves backend safe summary for process errors + adds unit test. |
| crates/ironclaw_events/tests/durable_log_contract.rs | Updates durable log contract fixture to include error_summary. |
| crates/ironclaw_events/src/runtime_event.rs | Adds error_summary to runtime events with sanitization and builder helper. |
| crates/ironclaw_event_streams/tests/event_stream_manager_contract/support/builders.rs | Updates event-stream test builders to include error_summary. |
| crates/ironclaw_event_streams/src/types.rs | Extends live projection item variant with optional error_summary. |
| crates/ironclaw_event_projections/tests/replay_projection_contract.rs | Updates replay-projection tests to include error_summary in runtime events. |
| crates/ironclaw_event_projections/src/runtime_projection.rs | Projects/clears error_summary in capability activity projection updates. |
| crates/ironclaw_event_projections/src/lib.rs | Extends CapabilityActivityProjection struct with optional error_summary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// `Ok(None)` if the manifest is valid but lacks a package name, allowing | ||
| /// callers to fall back to the extension name in that case. |
| fn truncate_keeps_short_values_intact() { | ||
| assert_eq!(truncate_env_value_for_display("12"), "12"); | ||
| let out = truncate_env_value_for_display("12"); | ||
| assert!(out == "12"); // safety: truncation contract assertion in test module. | ||
| } |
| // 64 chars + the ellipsis. | ||
| assert_eq!(out.chars().count(), 65); | ||
| let char_count = out.chars().count(); | ||
| assert!(char_count == 65); // safety: truncation contract assertion in test module. | ||
| } |
| let capability_id = tool_definition.capability_id.as_str(); | ||
| assert!(capability_id == "external_tool.client_lookup"); // safety: provider-name contract assertion. | ||
|
|
| let capability_id = candidate.capability_id.as_str(); | ||
| assert!(capability_id == "external_tool.client_lookup"); // safety: provider-call staging assertion. | ||
| } |
| let pending_count = pending_approval_count(local_runtime, &context).await; | ||
| if pending_count != 0 { | ||
| panic!("default-on auto-approve must not create a pending approval"); | ||
| } | ||
| assert!(pending_count == 0); // safety: test-only assertion in #[cfg(test)] module. | ||
| } |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 6ba9e77059b2 |
Head: 6ba9e77059b241eea6500fa8d691c8a10fc12ea3
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found one blocking redaction issue in the new error_summary projection path.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [HIGH] Re-sanitize error_summary before projecting it
Location: crates/ironclaw_event_projections/src/runtime_projection.rs:329
error_summary is copied directly from RuntimeEvent into CapabilityActivityProjection. Unlike error_kind, this skips the projection-boundary redaction required for custom/directly constructed runtime events. The existing regression test in replay_projection_contract.rs documents that public RuntimeEvent fields and custom DurableEventLog backends can bypass constructors and must be re-sanitized before product DTOs are serialized; setting error_summary to a raw value such as /tmp/private-host-path SECRET_PROJECTION_SENTINEL sk-live... would now appear in the serialized snapshot. Drop or validate the summary with the same safe-summary guard before assigning it here and in the update path, and extend that regression test to cover error_summary.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/failed/superseded state while reviewers run.
| process_id: event.process_id, | ||
| output_bytes: event.output_bytes, | ||
| error_kind: event.error_kind.clone().map(sanitize_error_kind), | ||
| error_summary: event.error_summary.clone(), |
There was a problem hiding this comment.
This new field needs the same projection-boundary redaction as error_kind. A custom/directly constructed RuntimeEvent can carry an unsafe error_summary, and this clone will put raw paths/tokens into serialized projection DTOs. Please validate/drop the summary here and in the update path, and add it to the existing custom-backend redaction regression.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/ironclaw_turns/src/run_profile/milestones.rs (1)
496-511: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not discard
safe_summaryin the emitter.
LoopHostMilestoneKind::CapabilityFailednow carries a summary, butLoopHostMilestoneEmitter::capability_failedalways sets it toNoneand provides no way for callers to supply one. The downstream conversion incrates/ironclaw_reborn/src/milestone_events.rs:237-260therefore receives no summary on the standard path.Thread
Option<LoopSafeSummary>through this method and test the real emitter-to-runtime-event path.As per path instructions, test propagation through the caller rather than only the enum or mapper.
🤖 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/ironclaw_turns/src/run_profile/milestones.rs` around lines 496 - 511, LoopHostMilestoneEmitter::capability_failed currently discards the CapabilityFailed safe_summary. Add an Option<LoopSafeSummary> parameter, pass it into the emitted LoopHostMilestoneKind::CapabilityFailed value, update all callers, and add an integration-style test exercising the real emitter-to-runtime-event path to verify the summary reaches the downstream event conversion.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/ironclaw_event_projections/src/runtime_projection.rs`:
- Around line 306-308: The projection currently copies event.error_summary
without sanitization, allowing paths or secrets into surfaced projections. In
the status handling within the runtime projection materialization logic, pass
event.error_summary through the canonical validated/redacted error-summary
sanitizer before assigning activity.error_summary, preserving None and only
applying it for Failed events. Add a regression test covering path and token
input to verify the projected summary contains neither raw secret nor host-path
data.
In `@crates/ironclaw_events/src/runtime_event.rs`:
- Line 170: Make error_summary a truly redacted event-boundary value across the
constructors, builders, and serialization paths including
sanitize_error_summary, with_error_summary, and the additionally referenced call
sites. Replace the current control-character stripping/truncation with an
event-owned validated summary type or strict rejection/collapse of unsafe path,
token/API-key-shaped, backend/provider, and other sensitive content before
serialization. Add durable serialization tests proving path and token-shaped
inputs cannot persist or emit unredacted content.
---
Outside diff comments:
In `@crates/ironclaw_turns/src/run_profile/milestones.rs`:
- Around line 496-511: LoopHostMilestoneEmitter::capability_failed currently
discards the CapabilityFailed safe_summary. Add an Option<LoopSafeSummary>
parameter, pass it into the emitted LoopHostMilestoneKind::CapabilityFailed
value, update all callers, and add an integration-style test exercising the real
emitter-to-runtime-event path to verify the summary reaches the downstream event
conversion.
🪄 Autofix (Beta)
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: f03adeb6-9ce6-4672-be2e-f8fc25880bdc
📒 Files selected for processing (33)
crates/ironclaw_event_projections/src/lib.rscrates/ironclaw_event_projections/src/runtime_projection.rscrates/ironclaw_event_projections/tests/replay_projection_contract.rscrates/ironclaw_event_streams/src/types.rscrates/ironclaw_event_streams/tests/event_stream_manager_contract/support/builders.rscrates/ironclaw_events/src/runtime_event.rscrates/ironclaw_events/tests/durable_log_contract.rscrates/ironclaw_host_runtime/src/first_party_tools/shell.rscrates/ironclaw_loop_support/src/capability_port.rscrates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rscrates/ironclaw_product_adapters/src/outbound.rscrates/ironclaw_reborn/src/milestone_events.rscrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/tests/loop_milestone_event_projection.rscrates/ironclaw_reborn_cli/src/operator_env.rscrates/ironclaw_reborn_composition/src/factory/local_dev_host_tests/approval_gates.rscrates/ironclaw_reborn_composition/src/nearai_mcp.rscrates/ironclaw_reborn_composition/src/projection.rscrates/ironclaw_reborn_composition/src/projection/display_preview.rscrates/ironclaw_reborn_composition/src/projection/live_progress.rscrates/ironclaw_reborn_composition/src/projection/tests/display_preview.rscrates/ironclaw_reborn_composition/src/projection/tests/display_preview_runtime.rscrates/ironclaw_reborn_composition/src/projection/tests/live_progress_stream.rscrates/ironclaw_reborn_composition/src/projection/tests/runtime_stream.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rscrates/ironclaw_reborn_webui_ingress/src/lib.rscrates/ironclaw_reborn_webui_ingress/tests/serve_loop.rscrates/ironclaw_triggers/tests/repository_contract.rscrates/ironclaw_turns/src/run_profile/milestones.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rssrc/extensions/manager.rs
💤 Files with no reviewable changes (1)
- src/extensions/manager.rs
| if matches!(status, CapabilityActivityStatus::Failed) && event.error_summary.is_some() { | ||
| activity.error_summary = event.error_summary.clone(); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Re-sanitize error_summary before materializing projections.
Unlike error_kind, event.error_summary is copied directly. A custom or legacy backend can provide an unsanitized RuntimeEvent, allowing sensitive text to reach CapabilityActivityProjection and live projections. Use the canonical validated/redacted summary boundary during replay and add a regression covering path/token input.
As per path instructions, projection error details must be sanitized during replay and raw secrets or host paths must not enter externally surfaced projections.
Also applies to: 329-329
🤖 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/ironclaw_event_projections/src/runtime_projection.rs` around lines 306
- 308, The projection currently copies event.error_summary without sanitization,
allowing paths or secrets into surfaced projections. In the status handling
within the runtime projection materialization logic, pass event.error_summary
through the canonical validated/redacted error-summary sanitizer before
assigning activity.error_summary, preserving None and only applying it for
Failed events. Add a regression test covering path and token input to verify the
projected summary contains neither raw secret nor host-path data.
Source: Path instructions
| process_id: self.process_id, | ||
| output_bytes: self.output_bytes, | ||
| error_kind: self.error_kind.clone().map(sanitize_error_kind), | ||
| error_summary: self.error_summary.clone().and_then(sanitize_error_summary), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Make error_summary a genuinely redacted boundary value.
sanitize_error_summary preserves paths, bearer tokens, API-key-like values, and backend/provider text; it only strips controls and truncates. Because direct construction and with_error_summary can feed this value into event serialization, sensitive content can be persisted or emitted before downstream product validation.
Use an event-owned validated summary type or reject/collapse unsafe input at this boundary, with durable serialization tests for path and token-shaped values.
As per path instructions, events and audit records are redacted by contract; raw secrets, host paths, and unredacted content must not enter ironclaw_events.
Also applies to: 266-266, 291-291, 632-632, 678-680, 827-850
🤖 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/ironclaw_events/src/runtime_event.rs` at line 170, Make error_summary
a truly redacted event-boundary value across the constructors, builders, and
serialization paths including sanitize_error_summary, with_error_summary, and
the additionally referenced call sites. Replace the current control-character
stripping/truncation with an event-owned validated summary type or strict
rejection/collapse of unsafe path, token/API-key-shaped, backend/provider, and
other sensitive content before serialization. Add durable serialization tests
proving path and token-shaped inputs cannot persist or emit unredacted content.
Source: Path instructions
|
🚅 Deployed to the ironclaw-pr-5962 environment in ironclaw-ci-preview
|
Summary
Split 1 of 3 extracted from #5279 (
codex/reborn-queued-messages-webui).This is the base of a stacked series — merge in order: this → queued-steering → budget-gate.
Robustness / hardening / test-hygiene work that is not tied to the queued-message or budget features:
error_summary/safe_summarysanitization surfaced end-to-end:error_summaryadded acrossruntime_event, event projections, event streams; re-validated at the product-adapter boundary (validate_optional_safe_summary+SAFE_SUMMARY_FORBIDDEN_MARKERS), host_runtime shell safe-summary, milestone/failure-summary plumbing.model_gatewaychar-boundary/bounds guards, milestone events.reborn_cli/operator_env, local-dev tests).Notes
main(so the diff shown againstmainis exactly this slice). The three stacked branches together reproduce [codex] fix Reborn queued message steering #5279's tree byte-for-byte.cargo check --tests --features webui-v2-betagreen for the affected crates.🤖 Generated with Claude Code