feat(worker): harden reasoning streams and sanitize job event payloads - #461
panosAthDBX wants to merge 5 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the worker and orchestrator's event handling and safety mechanisms. It introduces a comprehensive system for sanitizing and redacting sensitive information within job event payloads, ensuring data privacy and compliance. Additionally, it enhances the observability of worker operations by providing structured reasoning events and more detailed job result metadata. The changes also bolster the stability of LLM integrations by gracefully handling provider-level panics. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly hardens the worker and orchestrator by introducing sanitization and redaction for job event payloads, which is a great security improvement. The addition of structured reasoning events with stable parallel-group numbering is also a valuable enhancement for observability. The code is well-structured and the changes are well-tested. I've found one minor issue regarding redundant sanitization which could be optimized.
| input: crate::tools::redaction::redact_sensitive_json( | ||
| payload | ||
| .data | ||
| .get("input") | ||
| .unwrap_or(&serde_json::Value::Null), | ||
| ), |
There was a problem hiding this comment.
The payload.data is already sanitized on line 303 by sanitize_job_event_data, which calls redact_sensitive_json for tool_use events. This second call to redact_sensitive_json is redundant and introduces unnecessary overhead from cloning and traversing the JSON value again.
You can simplify this by directly using the already-sanitized value from payload.data.
| input: crate::tools::redaction::redact_sensitive_json( | |
| payload | |
| .data | |
| .get("input") | |
| .unwrap_or(&serde_json::Value::Null), | |
| ), | |
| input: payload | |
| .data | |
| .get("input") | |
| .cloned() | |
| .unwrap_or(serde_json::Value::Null), |
zmanian
left a comment
There was a problem hiding this comment.
Review: PR #461 -- feat(worker): harden reasoning streams and sanitize job event payloads
Reviewed against IronClaw standards. This PR delivers security-critical sanitization of job event payloads flowing through the orchestrator, plus structured reasoning event emission from workers.
Positive observations
- No
.unwrap()/.expect()in production code -- all instances are in test modules only. Clean. - Job event sanitization (
sanitize_job_event_data) is correctly placed: it runs before both persistence and broadcast, covering both vectors. Thetool_usepath redacts sensitive JSON keys ininput, and thetool_resultpath runssafety.sanitize_tool_output()on output text. This is the right approach. - Worker-side sanitization:
sanitize_worker_narrative()andsanitize_worker_rationale()mirror the agent-side patterns from PR #460, ensuring consistency. Both fall back safely (None for narratives,DEFAULT_TOOL_RATIONALEfor rationales) when blocked. - Worker tool output now runs through
safety.sanitize_tool_output()instead of rawtruncate()-- significant security improvement for job event streams. - Worker tool input now uses
redact_sensitive_json()instead oftruncate(&tc.arguments.to_string(), 500)-- prevents credential leakage in tool_use events. SseEvent::JobResultnow carriessuccessandmessagefields -- good structured enrichment that avoids ambiguous status strings.- Rig adapter panic guard (
run_completion_guarded) is a solid defensive addition -- catches knownrig-coreusage underflow panics, marks the adapter as poisoned, and converts panics to typedLlmErrors. TheAssertUnwindSafeusage is documented with a safety comment. - Test coverage:
job_event_redacts_tool_use_input,job_event_sanitizes_tool_result_output,job_event_handles_reasoning_event,job_event_handles_result_event, worker parallel group monotonicity -- all the security-critical paths are tested.
Minor notes (non-blocking)
- The
OrchestratorStatenow requires asafety: Arc<SafetyLayer>field -- themain.rschange correctly wires this. All test constructors also updated. Clean. - The worker completion state machine change (removing
mark_completedlogic from theOk(Ok(()))branch inworker.rs) is a behavioral change worth noting -- the worker no longer marks itself completed on success. Ensure the caller/job monitor handles this transition. - The
normalize_tool_reasoning("").into_owned()pattern appears frequently in test code -- could be a helper, but not worth changing.
LGTM. Approving.
|
Closing — the source branch has been deleted and can no longer be merged. If this feature is still needed, please open a fresh PR from a new branch. |
Pull request was closed
Summary
job_resultevents with success/message metadata and update web event typings/SSE routing accordinglyWhy this split
Reviewer feedback on #361 asked for decomposition into focused PRs. This PR isolates the worker/orchestrator streaming and safety hardening layer.
Scope
src/worker/runtime.rs)src/orchestrator/api.rs)src/channels/web/types.rs,src/channels/web/sse.rs)src/agent/worker.rs,src/agent/job_monitor.rs,src/main.rs,src/safety/mod.rsValidation
Ran locally on this branch:
cargo fmt --checkcargo clippy --all --all-featurescargo testAll passed.