[Reborn] Wire SkillContextService into loop prompt path - #3476
serrrfirat merged 4 commits into
Conversation
github-actions
Bot
added
scope: dependencies
![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 support for skill instruction snippets within the agent loop by adding a HostSkillContextSource trait and integrating it into the context and model ports. The changes allow for parsing skill markdown, applying visibility and trust-based filtering, and materializing these snippets as system messages. Feedback suggests using .as_ref() when handling Option fields in the host factory to improve code robustness and avoid unnecessary cloning or potential move issues.
| if let Some(source) = self.skill_context_source.clone() { | ||
| context_adapter = context_adapter.with_skill_context_source(source); | ||
| } |
There was a problem hiding this comment.
Use .as_ref() on Option fields within a struct to prevent partial moves, making the code more robust against future changes.
| if let Some(source) = self.skill_context_source.clone() { | |
| context_adapter = context_adapter.with_skill_context_source(source); | |
| } | |
| if let Some(source) = self.skill_context_source.as_ref() { | |
| context_adapter = context_adapter.with_skill_context_source(source.clone()); | |
| } |
References
- Use .as_ref().map() on Option fields within a struct to prevent partial moves, making the code more robust against future changes.
| if let Some(source) = self.skill_context_source.clone() { | ||
| model_adapter = model_adapter.with_skill_context_source(source); | ||
| } |
There was a problem hiding this comment.
Use .as_ref() on Option fields within a struct to prevent partial moves, making the code more robust against future changes.
| if let Some(source) = self.skill_context_source.clone() { | |
| model_adapter = model_adapter.with_skill_context_source(source); | |
| } | |
| if let Some(source) = self.skill_context_source.as_ref() { | |
| model_adapter = model_adapter.with_skill_context_source(source.clone()); | |
| } |
References
- Use .as_ref().map() on Option fields within a struct to prevent partial moves, making the code more robust against future changes.
7e74526 to
cdb676c
Compare
cdb676c to
c885be8
Compare
ReviewSummary: Wires Findings:
Minor: FNV-1a is fine for dedup but a 64-bit non-cryptographic hash means a malicious skill author who controls Verdict: Request changes — (1) and (4) are blockers; (2) and (3) need explicit answers. |
github-actions
Bot
added
size: XL
…omments-20260511204831 # Conflicts: # crates/ironclaw_turns/src/run_profile/skill_context.rs
Post-merge re-review3 of 5 prior findings resolved cleanly, 1 deferred to #3540, but 1 blocker still open and 1 new gap introduced: Resolved
Still open
Filing a follow-up issue for both items. Recommend not relying on the skill-context path in production traffic until both are closed. |
…ntext-prod-20260511153002 [Reborn] Wire SkillContextService into loop prompt path
2 participants
Refs #3473
Stacked on #3470 (
kb/kb-007).Summary
HostSkillContextSourceand parser-backed snapshot builder inironclaw_loop_support.ironclaw_skills::parse_skill_mdandSkillTrustto buildSkillRunSnapshotforSkillContextService.RebornLoopDriverHostFactory::with_skill_context_source(...)so production composition can inject the real skill source without moving parsing intoironclaw_turns.Tests
CARGO_TARGET_DIR=/tmp/ironclaw-target-3473 cargo fmt --all -- --checkCARGO_TARGET_DIR=/tmp/ironclaw-target-3473 cargo test -p ironclaw_loop_support --quietCARGO_TARGET_DIR=/tmp/ironclaw-target-3473 cargo test -p ironclaw_turns --quietCARGO_TARGET_DIR=/tmp/ironclaw-target-3473 cargo test -p ironclaw_reborn --quietCARGO_TARGET_DIR=/tmp/ironclaw-target-3473 cargo test -p ironclaw_architecture --quietCARGO_TARGET_DIR=/tmp/ironclaw-target-3473 cargo clippy -p ironclaw_turns -p ironclaw_loop_support -p ironclaw_reborn --all-targets -- -D warningsNotes
ironclaw_turnsas snapshot/prompt contract layer.HostSkillContextSourceimplementation for the active Reborn skill store.