fix(reborn): hide routine implementation details - #6038
italic-jinxin wants to merge 15 commits into
Conversation
italic-jinxin
added
size: XL
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRoutine trigger prompts are embedded from Markdown, routine previews use bounded allowlisted summaries, and capability-owned final replies flow through the loop before transcript finalization. Tests cover redaction, persistence, malformed fields, lifecycle handling, and integration behavior. ChangesRoutine presentation and final-reply boundary
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TriggerManagement
participant RoutinePresentation
participant CapabilityWriteResult
participant AgentLoop
participant FinalTranscript
TriggerManagement->>RoutinePresentation: project routine result
RoutinePresentation-->>TriggerManagement: bounded preview and safe reply
TriggerManagement->>CapabilityWriteResult: attach safe reply
CapabilityWriteResult->>AgentLoop: forward result reference
AgentLoop->>FinalTranscript: apply safe reply before finalization
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
github-actions
Bot
added
size: L
github-actions
Bot
added
size: L
![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 transitions user-facing terminology from 'trigger' to 'routine' and ensures that internal implementation details, such as raw cron expressions and internal IDs, are redacted from user-facing replies and previews. It updates capability descriptions, display preview formatting, test assertions, and LLM trace fixtures to enforce this boundary. Feedback was provided regarding a potential discrepancy in routine_list_preview_lines where the reported count of routines could mismatch the actual listed routines if any are skipped due to missing names; a suggestion was made to filter the triggers first.
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.
|
@claude review |
This comment was marked as resolved.
This comment was marked as resolved.
|
🚅 Deployed to the ironclaw-pr-6038 environment in ironclaw-ci-preview
|
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.57% — 302718 / 353765 lines Per-crate breakdown (63 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
|
@claude review |
![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_reborn_composition/src/projection/display_preview.rs (1)
917-1210: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep routine list counts consistent with rendered entries.
routine_list_preview_linesderives the count, capacity, and overflow marker fromtriggers.len(), but skips records without a non-emptyname. A malformed record can therefore produce"2 routines found"while rendering one row; ten malformed records before a valid one can also hide the valid routine behind the limit.Filter to displayable named routines first, then derive the count and limit from that collection (or render an explicit safe placeholder). Add a caller-level regression test for malformed list entries.
🤖 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_reborn_composition/src/projection/display_preview.rs` around lines 917 - 1210, Update routine_list_preview_lines to filter out triggers lacking a non-empty name before computing the displayed count, capacity, overflow marker, and ROUTINE_LIST_PREVIEW_LIMIT slice, so counts and rendered rows remain consistent and valid routines are not hidden by malformed entries. Add a caller-level regression test covering malformed list records, including entries before a valid routine.
🤖 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_reborn_composition/src/projection/display_preview.rs`:
- Around line 917-1210: Update routine_list_preview_lines to filter out triggers
lacking a non-empty name before computing the displayed count, capacity,
overflow marker, and ROUTINE_LIST_PREVIEW_LIMIT slice, so counts and rendered
rows remain consistent and valid routines are not hidden by malformed entries.
Add a caller-level regression test covering malformed list records, including
entries before a valid routine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f0fc9ffd-e9ea-4560-bcc6-e22b68b96419
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/projection/display_preview.rs
…ne-internals # Conflicts: # crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs # crates/ironclaw_host_runtime/tests/tool_surface_contract.rs
There was a problem hiding this comment.
A few blocking correctness issues remain:
-
P2 -
routine_capability()atdisplay_preview.rs:967-975classifies by namespace-stripped suffix. A valid third-party capability such asacme.trigger_createis therefore presented as a built-in Routine, and its real/custom preview is replaced. Please exact-match the canonical first-party IDs and supported provider aliases, with a negative third-party test. -
P2 - final-reply redaction does not cover every routine path. Only create/list descriptions gained the presentation constraint, while pause/resume/remove remain generic and
trigger_output()still gives the model IDs, raw schedules, and host metadata. The routine skill is not guaranteed to activate for direct pause/delete/disable wording. Please provide the safe presentation contract for all five verbs. -
The recorded QA assertions replay manually edited canned final text, so they do not demonstrate that the new prompts produce safe replies. Please re-record under the new prompt and add caller-level coverage that attempts to return an ID/raw cron and verifies the product boundary.
-
P2 -
trigger_list.mdasks the model to summarize tasks, buttrigger_output()provides no task or safe task summary. The model must guess from the name. Either expose a bounded user-facing task summary or remove that requirement. -
P3 -
routine_list_preview_lines()counts and truncates the raw array before skipping nameless entries. This can report 12 routines while rendering fewer, and malformed leading entries can hide valid later entries. Filter displayable entries before count/limit calculations.
github-actions
Bot
added
size: XL
github-actions
Bot
added
size: L
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_host_api/src/dispatch.rs`:
- Around line 59-81: Update CapabilityFinalReplyPresentation to enforce
validation during deserialization by removing direct Deserialize derivation and
using serde’s try_from = "String" with its fallible new constructor. Rename
safe_reply() to the required as_str() accessor, then update downstream callers
such as assistant_reply.rs to use as_str() while preserving trimming,
truncation, and empty-value rejection.
In
`@crates/ironclaw_loop_host/src/capability_port/tests/runtime_lifecycle_tests.rs`:
- Around line 174-188: In the assertion for the extracted final-reply
presentation, replace the safe_reply() call with as_str() to match the newtype
validation API introduced in dispatch.rs, while preserving the expected "Edited
1 file" value.
🪄 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: f7b44e47-2675-462c-bd10-68859eb095ab
📒 Files selected for processing (37)
crates/ironclaw_agent_loop/src/executor.rscrates/ironclaw_agent_loop/src/executor/assistant_reply.rscrates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/loop_exit.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_architecture/tests/routine_presentation_boundary.rscrates/ironclaw_first_party_extensions/src/coding/diff_preview.rscrates/ironclaw_host_api/src/dispatch.rscrates/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/ironclaw_host_runtime/src/first_party_tools/prompts/trigger_list.mdcrates/ironclaw_host_runtime/src/first_party_tools/prompts/trigger_pause.mdcrates/ironclaw_host_runtime/src/first_party_tools/prompts/trigger_remove.mdcrates/ironclaw_host_runtime/src/first_party_tools/prompts/trigger_resume.mdcrates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rscrates/ironclaw_host_runtime/src/first_party_tools/trigger_presentation.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/obligations.rscrates/ironclaw_loop_host/src/capability_port.rscrates/ironclaw_loop_host/src/capability_port/tests/runtime_lifecycle_tests.rscrates/ironclaw_loop_host/src/subagent_spawn_port/tests.rscrates/ironclaw_loop_host/tests/thread_loop_host_contract.rscrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle_capabilities.rscrates/ironclaw_reborn_composition/src/projection/display_preview.rscrates/ironclaw_reborn_composition/src/projection/tests/display_preview.rscrates/ironclaw_reborn_composition/src/projection/tests/routine_display_preview.rscrates/ironclaw_reborn_composition/src/root/product_live_adapters.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/result_read.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests/tests/display_preview.rscrates/ironclaw_turns/src/run_profile/model_observation.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rsdocs/reborn/contracts/triggers.mdtests/integration/group_triggers/main.rstests/integration/group_triggers/scenario_final_reply_boundary.rstests/integration/support/golden.rstests/reborn_qa_recorded_behavior.rs
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct CapabilityFinalReplyPresentation { | ||
| safe_reply: String, | ||
| } | ||
|
|
||
| impl CapabilityFinalReplyPresentation { | ||
| pub fn new(safe_reply: impl Into<String>) -> Option<Self> { | ||
| let safe_reply = truncate_capability_display_text( | ||
| safe_reply.into().trim(), | ||
| CAPABILITY_FINAL_REPLY_MAX_BYTES, | ||
| ) | ||
| .text; | ||
| if safe_reply.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| Some(Self { safe_reply }) | ||
| } | ||
|
|
||
| pub fn safe_reply(&self) -> &str { | ||
| &self.safe_reply | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce newtype validation on deserialization.
CapabilityFinalReplyPresentation derives Deserialize directly, allowing storage or network payloads to bypass the new() constructor and deserialize empty or oversized strings. As per coding guidelines, validated newtypes must use #[serde(try_from = "String")], a fallible new, and explicit as_str() methods.
Note: You will also need to update downstream callers (e.g., assistant_reply.rs) to use .as_str() instead of .safe_reply().
🛡️ Proposed fix to secure the deserialization boundary
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
-pub struct CapabilityFinalReplyPresentation {
- safe_reply: String,
-}
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(try_from = "String", into = "String")]
+pub struct CapabilityFinalReplyPresentation(String);
impl CapabilityFinalReplyPresentation {
pub fn new(safe_reply: impl Into<String>) -> Option<Self> {
let safe_reply = truncate_capability_display_text(
safe_reply.into().trim(),
CAPABILITY_FINAL_REPLY_MAX_BYTES,
)
.text;
if safe_reply.is_empty() {
return None;
}
- Some(Self { safe_reply })
+ Some(Self(safe_reply))
}
- pub fn safe_reply(&self) -> &str {
- &self.safe_reply
+ pub fn as_str(&self) -> &str {
+ &self.0
}
}
+
+impl TryFrom<String> for CapabilityFinalReplyPresentation {
+ type Error = &'static str;
+
+ fn try_from(value: String) -> Result<Self, Self::Error> {
+ Self::new(value).ok_or("Final reply presentation cannot be empty")
+ }
+}
+
+impl From<CapabilityFinalReplyPresentation> for String {
+ fn from(val: CapabilityFinalReplyPresentation) -> Self {
+ val.0
+ }
+}🤖 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_host_api/src/dispatch.rs` around lines 59 - 81, Update
CapabilityFinalReplyPresentation to enforce validation during deserialization by
removing direct Deserialize derivation and using serde’s try_from = "String"
with its fallible new constructor. Rename safe_reply() to the required as_str()
accessor, then update downstream callers such as assistant_reply.rs to use
as_str() while preserving trimming, truncation, and empty-value rejection.
Source: Coding guidelines
| let CapabilityOutcome::Completed(completed) = outcome else { | ||
| panic!("expected completed outcome"); | ||
| }; | ||
| let presentation = match completed | ||
| .model_observation | ||
| .as_ref() | ||
| .map(|observation| &observation.detail) | ||
| { | ||
| Some(ironclaw_turns::run_profile::ToolObservationDetail::ResultReference { | ||
| final_reply_presentation: Some(presentation), | ||
| .. | ||
| }) => presentation, | ||
| detail => panic!("expected final-reply presentation, got {detail:?}"), | ||
| }; | ||
| assert_eq!(presentation.safe_reply(), "Edited 1 file"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Update method call to align with newtype validation guidelines.
If you apply the recommended newtype fix in crates/ironclaw_host_api/src/dispatch.rs, update this assertion to use .as_str() instead of .safe_reply().
♻️ Proposed refactor
};
- assert_eq!(presentation.safe_reply(), "Edited 1 file");
+ assert_eq!(presentation.as_str(), "Edited 1 file");
let previews = result_writer.display_previews();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let CapabilityOutcome::Completed(completed) = outcome else { | |
| panic!("expected completed outcome"); | |
| }; | |
| let presentation = match completed | |
| .model_observation | |
| .as_ref() | |
| .map(|observation| &observation.detail) | |
| { | |
| Some(ironclaw_turns::run_profile::ToolObservationDetail::ResultReference { | |
| final_reply_presentation: Some(presentation), | |
| .. | |
| }) => presentation, | |
| detail => panic!("expected final-reply presentation, got {detail:?}"), | |
| }; | |
| assert_eq!(presentation.safe_reply(), "Edited 1 file"); | |
| let CapabilityOutcome::Completed(completed) = outcome else { | |
| panic!("expected completed outcome"); | |
| }; | |
| let presentation = match completed | |
| .model_observation | |
| .as_ref() | |
| .map(|observation| &observation.detail) | |
| { | |
| Some(ironclaw_turns::run_profile::ToolObservationDetail::ResultReference { | |
| final_reply_presentation: Some(presentation), | |
| .. | |
| }) => presentation, | |
| detail => panic!("expected final-reply presentation, got {detail:?}"), | |
| }; | |
| assert_eq!(presentation.as_str(), "Edited 1 file"); | |
| let previews = result_writer.display_previews(); |
🤖 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_loop_host/src/capability_port/tests/runtime_lifecycle_tests.rs`
around lines 174 - 188, In the assertion for the extracted final-reply
presentation, replace the safe_reply() call with as_str() to match the newtype
validation API introduced in dispatch.rs, while preserving the expected "Edited
1 file" value.
There was a problem hiding this comment.
Re-reviewed the latest head. The current tree is identical to the previously reviewed 0ea00e862 tree, so the functional findings in my earlier review remain current: third-party suffix misclassification, incomplete mutation-path redaction, canned rather than boundary-level final-reply coverage, missing task data for list summaries, and malformed list count/limit handling. Not approving this head yet.
hanakannzashi
left a comment
hanakannzashi
left a comment
There was a problem hiding this comment.
Approved after follow-up review. The remaining discussion items are non-blocking for this change.
This branch was successfully deployed
2 participants
Summary
Linked Issue
Closes #5707
Validation
cargo test -p ironclaw_architecturecargo test -p ironclaw_host_runtime --test tool_surface_contractcargo test -p ironclaw_reborn_composition --libcargo test --test reborn_qa_recorded_behavior contract_routine_ -- --nocapturecargo test --test reborn_qa_recorded_behavior replay_routine_ -- --nocapturescripts/ci/check-reborn-qa-fixtures.sh-D warningscargo fmt --all -- --checkSecurity Impact
Reduces accidental disclosure of internal routine identifiers, raw schedules, stored prompts, capability names, and host metadata through activity previews and assistant replies.
Database Impact
None. No schema or persistence-contract changes.
Blast Radius
Limited to Reborn routine-management display previews, model-facing routine guidance, and recorded QA expectations.
Rollback Plan
Revert the two commits to restore the previous generic capability previews and routine response guidance.
Review track: B