feat(reborn): support Reborn operator log tail/follow - #4804
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a ChangesOperator Logs tail/follow + path redaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
scope: docs
![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 implements tail and follow functionality for operator logs, enabling clients to retrieve the latest logs chronologically and stream newer entries using cursors. It also adds sensitive host path redaction to log messages. The review feedback identifies an asymmetrical trimming bug in the path redaction logic where the opening curly brace { is omitted, potentially bypassing redaction in structured logs, and suggests using std::borrow::Cow to optimize memory allocation during token scanning.
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.
think-in-universe
left a comment
think-in-universe
left a comment
There was a problem hiding this comment.
I found one issue in the new log path redaction behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_product_workflow/src/reborn_services.rs (1)
3851-3864:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject conflicting log pagination modes (
tail+follow) at the facade boundary.Line [3862] and Line [3863] currently pass both flags through unchanged. That creates ambiguous semantics for a single request mode and silently depends on backend precedence (
followcurrently wins). Enforce mutual exclusivity and return a typed validation error when both aretrue.Suggested fix
fn bounded_operator_logs_query(query: RebornOperatorLogsQuery) -> RebornLogQueryRequest { + // Fail loud on conflicting pagination modes. + // tail=true => latest window; follow=true => increment after cursor. + // Both together is ambiguous and should be rejected by caller-facing validation. RebornLogQueryRequest { limit: Some( query .limit .unwrap_or(OPERATOR_LOGS_DEFAULT_LIMIT) .clamp(1, OPERATOR_LOGS_MAX_LIMIT), ), cursor: bounded_operator_logs_string(query.cursor, OPERATOR_LOGS_CURSOR_MAX_BYTES), level: query.level, target: bounded_operator_logs_string(query.target, OPERATOR_LOGS_TARGET_MAX_BYTES), tail: query.tail, follow: query.follow, } }async fn query_operator_logs( &self, caller: WebUiAuthenticatedCaller, query: RebornOperatorLogsQuery, ) -> Result<RebornOperatorCommandPlaneResponse, RebornServicesError> { + if query.tail && query.follow { + return Err(RebornServicesError::validation(WebUiInboundValidationError::new( + "follow", + WebUiInboundValidationCode::InvalidValue, + ))); + } let request = bounded_operator_logs_query(query); let logs = self.operator_logs.query_logs(caller, request).await?; Ok(RebornOperatorCommandPlaneResponse {Based on learnings and invariants: “Fail loud: flag silent-failure patterns … errors propagate with context.”
🤖 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_product_workflow/src/reborn_services.rs` around lines 3851 - 3864, The bounded_operator_logs_query function currently passes both tail and follow flags through without validation, creating ambiguous semantics when both are true. Add validation logic to reject requests where both tail and follow are true by returning a typed validation error instead of constructing the RebornLogQueryRequest. Change the function's return type from RebornLogQueryRequest to a Result type that can represent validation failures, and implement the check before building the response struct to enforce mutual exclusivity of these conflicting pagination modes.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.
Outside diff comments:
In `@crates/ironclaw_product_workflow/src/reborn_services.rs`:
- Around line 3851-3864: The bounded_operator_logs_query function currently
passes both tail and follow flags through without validation, creating ambiguous
semantics when both are true. Add validation logic to reject requests where both
tail and follow are true by returning a typed validation error instead of
constructing the RebornLogQueryRequest. Change the function's return type from
RebornLogQueryRequest to a Result type that can represent validation failures,
and implement the check before building the response struct to enforce mutual
exclusivity of these conflicting pagination modes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a6ed549-fdce-456a-a81c-158b0591167a
📒 Files selected for processing (7)
crates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/reborn_services/types.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/operator_logs.rscrates/ironclaw_webui_v2_static/static/js/lib/api.jsdocs/reborn/contracts/operator-observability-backends.md
think-in-universe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_product_workflow/src/reborn_services.rs`:
- Around line 3934-3935: The `bounded_operator_logs_query` function is setting
both `tail` and `follow` fields from the query without validation, but these
represent mutually exclusive query modes according to the contract. Add
validation logic to reject requests where both `tail` and `follow` are set to
`true` simultaneously, returning an appropriate error response to the caller
when this ambiguous combination is detected.
🪄 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: 33947b26-4053-4849-84ab-9431aa314c0f
📒 Files selected for processing (6)
crates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/reborn_services/types.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/operator_logs.rscrates/ironclaw_webui_v2_static/static/js/lib/api.js
💤 Files with no reviewable changes (1)
- crates/ironclaw_webui_v2_static/static/js/lib/api.js
think-in-universe
left a comment
think-in-universe
left a comment
There was a problem hiding this comment.
I reviewed the new commits on this PR. The main risks I see are around log redaction coverage, follow cursor behavior, and caller-level coverage/docs for the new public query flag.
|
Human final review guidance: focus on operator log API semantics and redaction. Please verify tail/follow mutual exclusion, follow cursor high-water advancement with filters, byte-budget pagination, slash/backslash host-path redaction before WebUI exposure, and handler/facade propagation of |
|
@claude review |
Code Review — PR #4804Found 8 issues across security, architecture, performance, and correctness. Architecture & Type Design[MEDIUM:85] — Mutually-exclusive boolean flags should be an enum
[MEDIUM:80] — Excessive code duplication in query branching
[LOW:60] — Path redaction duplicates safety module logic
Correctness[MEDIUM:75] — Follow cursor advances past filtered entries, making them unreachable on filter change
Performance[HIGH:85] — Expensive serialization in hot loop
[HIGH:80] — Repeated
[MEDIUM:75] — Multiple iterations over path segments
[MEDIUM:70] — Target filter lowercased on every iteration
Test NotesTest coverage for validation, follow/tail forwarding, and cursor semantics is comprehensive and correct. Documentation updates (CLAUDE.md, webui_v2/CLAUDE.md) are accurate. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_reborn_composition/src/operator_logs.rs`:
- Around line 417-477: The response_entry_bytes() function only accounts for
individual entry serialization, but the actual MAX_LOG_RESPONSE_BYTES limit is
checked against a JSON array which includes overhead from brackets and commas.
Fix this by accounting for JSON array overhead in the byte budget calculations
at each push site. In the unnamed first branch, Tail mode branch, and Page mode
branch where entries are added to the selected vector, update the byte
comparisons against MAX_LOG_RESPONSE_BYTES to include the overhead of the JSON
array structure (opening bracket, closing bracket, and commas between entries)
so the budget check matches what serde_json::to_vec would produce.
🪄 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: 59eed80c-fb46-4b7a-bc74-8c105e29e53e
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/operator_logs.rs
What changed
followquery flag beside the existingtailflag for Reborn operator logs.tail/followthrough the product workflow facade after applying existing limit/cursor/target bounds.tail=trueresponses with an opaque follow cursorfollow=true&cursor=...responses for newer retained entriesfollow_supported: truefor the concrete in-process backendtail/followparams in the WebUI v2 JS API helper and updates Reborn operator logging docs.Why
Sub-issue #4597 requires query, tail, and follow behavior for the canonical WebUI v2 operator logs API. The merged log backend already provided bounded query/filter/redaction basics, but it still reported follow as unsupported and product workflow discarded the tail flag before reaching the backend.
Validation
cargo fmtcargo +1.92.0 test -p ironclaw_product_workflow query_operator_logs_bounds_query_before_logs_service -- --nocapturecargo +1.92.0 test -p ironclaw_reborn_composition operator_logs --features webui-v2-beta -- --nocaptureRemaining scope
This does not add a separate CLI wrapper. Any retained CLI logs command should wrap this same service/API evidence rather than implementing another log path.
Refs #4597
Refs #4533