Skill lifecycle phase 3: reflection rides the persistence branch - #624
Merged
Conversation
Phase 3 of docs/design-docs/skill-lifecycle.md — the outcome-to-skill pump. One deliberate divergence from the doc: no separate reflection branch. The persistence branch is already the silent replay-fork this needs, so reflection rides it as a single unified pass (the shape the Hermes reference implementation runs in production). - Channel accumulates a reflection signal: a turn crossing min_tool_iterations tool calls, or a worker completing successfully. Cron conversations never reflect. - When the signal is set and the per-conversation cooldown allows, the next persistence branch spawns with skill_reflection: its prompt gains the reflection section and its toolset gains read_skill/skill_manage/ skills_list under WriteOrigin::Agent, so every phase-2 rail applies. The signal alone can spawn the pass; passes without it are unchanged. - Reflection prompt section: decide-first (no writes is the common case), the patch-over-create preference ladder, and the negative- capture bans (no environment flukes, no negative tool claims, no transient errors, no one-off narratives). - [skills.reflection] config: enabled / min_tool_iterations (10) / cooldown_secs (3600), defaults + per-agent override, hot-reloadable.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe change adds configurable skill reflection settings, tracks reflection eligibility during channel activity, and lets memory persistence branches update skills with conditional prompts and tool access. ChangesSkill reflection persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent/channel.rs (1)
3829-3855: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetain the reflection request when branch creation fails.
Lines 3832-3835 start the cooldown and clear the signal before
spawn_memory_persistence_branchsucceeds. If spawning fails, no reflection branch runs, but the next check is suppressed by the cleared signal and cooldown.Restore the prior reflection state on the error path, or commit the signal reset and cooldown only after the branch is created.
🤖 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 `@src/agent/channel.rs` around lines 3829 - 3855, Retain the pending reflection request when spawn_memory_persistence_branch fails. Move the reflection cooldown and reflection_signal reset from before the spawn into the successful Ok(branch_id) path, or restore both prior values in the Err(error) path, while preserving the existing branch insertion and logging behavior.
🤖 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 `@prompts/en/memory_persistence.md.j2`:
- Around line 81-84: Provide a supported reference-file write path for the
reflection persistence flow: in prompts/en/memory_persistence.md.j2 lines 81-84,
replace write_file with an implemented skill-management operation if one
supports writing references; otherwise, update src/tools.rs lines 912-939 to
register a constrained reference-file write capability only for
reflection-enabled persistence branches while preserving agent-origin
restrictions.
In `@src/agent/channel.rs`:
- Around line 2934-2956: Move tool-call counting in the turn-history block to
occur before apply_history_after_turn mutates or sanitizes guard, using the
net-new source history including PromptCancelled.chat_history. Keep
apply_history_after_turn responsible for applying sanitized history, then use
the pre-sanitization count for the reflection.min_tool_iterations check and
mark_reflection_signal.
- Around line 3422-3430: Update the successful worker-completion branch in the
surrounding channel handler so that, after
mark_reflection_signal("worker_completed") sets the reflection signal, it
schedules check_memory_persistence. Preserve the existing reflection.enabled
guard and normal persistence controls while ensuring persistence is checked even
when no later user message occurs.
- Around line 3773-3780: Update the early-return condition in the
reflection/persistence flow to return whenever persistence_enabled is false,
regardless of reflection_due. Only allow reflection_due to trigger processing
after the existing memory-persistence controls pass, preserving the behavior of
self.reflection_due() for enabled persistence.
---
Outside diff comments:
In `@src/agent/channel.rs`:
- Around line 3829-3855: Retain the pending reflection request when
spawn_memory_persistence_branch fails. Move the reflection cooldown and
reflection_signal reset from before the spawn into the successful Ok(branch_id)
path, or restore both prior values in the Err(error) path, while preserving the
existing branch insertion and logging behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4350e341-4b98-40d7-8090-bf9759c06550
📒 Files selected for processing (12)
prompts/en/memory_persistence.md.j2src/agent/channel.rssrc/agent/channel_dispatch.rssrc/agent/channel_history.rssrc/agent/ingestion.rssrc/api/agents.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/prompts/engine.rssrc/tools.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| self.message_count = 0; | ||
| self.last_persistence_at = std::time::Instant::now(); | ||
| if reflection_due { | ||
| self.last_reflection_at = Some(std::time::Instant::now()); |
Contributor
There was a problem hiding this comment.
This consumes the reflection signal and starts the cooldown before the branch actually spawns. If spawn_memory_persistence_branch fails, reflection is lost for the full cooldown without ever running. Consider moving the cooldown update and reflection_signal.store(false, ...) into the Ok arm.
Member
Author
There was a problem hiding this comment.
Fixed in 31365da — the cooldown start and signal clear moved into the Ok arm; a failed spawn leaves the signal set so the next check retries. (Same fix covers the outside-diff comment on lines 3829-3855.)
- tool iterations counted from the pre-sanitization turn history; PromptCancelled/MaxTurnsError use the chat_history carried in the error, so reply-tool turns no longer report zero tool calls - successful worker completion schedules a persistence check instead of leaving the reflection signal pending until the next user message - reflection rides the persistence pass and now obeys the same write controls: no reflection-only branch when persistence is disabled, interval is zero, or the conversation memory mode isn't Full - reflection signal and cooldown are consumed in the spawn Ok arm, so a failed spawn retries instead of losing the reflection for a cooldown - persistence prompt names skill_manage's write_file action rather than a bare write_file tool
This was referenced Aug 10, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-lands #623. It was stacked on the phase 2 branch and got merged into that branch just after #622 had already landed on main — so the reflection commit never reached main. Same diff, reviewed there; branch is based on exactly the tree #622 merged.
Original description and discussion: #623