refs(#2264): Phase 2 — wire FrozenPrefix::verify() into turn_loop#2514
refs(#2264): Phase 2 — wire FrozenPrefix::verify() into turn_loop#2514encyc wants to merge 2 commits into
Conversation
Adds a three-zone diagnostic layer alongside the existing PrefixStabilityManager::check_and_update(). On the first turn, freeze the PinnedPrefix baseline; on subsequent turns, verify the current system+tool state against the frozen baseline and log drift via tracing::debug!. Phase 2 is warn-only — no request refusal — auto-re-freezes on drift to keep subsequent turn comparisons meaningful. - Session: add frozen_prefix: Option<FrozenPrefix> field - turn_loop: import PinnedPrefix, insert verify block after check_and_update, before MessageRequest construction
There was a problem hiding this comment.
Code Review
This pull request implements the three-zone prefix contract (#2264) by freezing the baseline prefix on the first turn of the engine loop and verifying it on subsequent turns to detect drift. The frozen prefix state is stored in a new frozen_prefix field on the Session struct. Feedback on the changes highlights an inefficiency where active_tools is unconditionally cloned on every turn; it is recommended to use a read-only slice for verification and only clone the tools when constructing or updating the PinnedPrefix.
| let current_tools: Vec<crate::models::Tool> = active_tools.clone().unwrap_or_default(); | ||
|
|
||
| match &self.session.frozen_prefix { | ||
| Some(frozen) => { | ||
| if let Err(drift) = frozen.verify(&system_text, ¤t_tools) { | ||
| tracing::debug!( | ||
| target: "prefix_cache", | ||
| "three-zone drift: {drift}" | ||
| ); | ||
| let pinned = | ||
| PinnedPrefix::new(self.session.system_prompt.as_ref(), current_tools); | ||
| self.session.frozen_prefix = Some(pinned.freeze()); | ||
| } | ||
| } | ||
| None => { | ||
| let pinned = | ||
| PinnedPrefix::new(self.session.system_prompt.as_ref(), current_tools); | ||
| self.session.frozen_prefix = Some(pinned.freeze()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unconditionally cloning active_tools on every turn of the engine loop is inefficient. Since Tool definitions can be large (containing descriptions, JSON schemas, etc.), we should avoid cloning them unless we actually need to freeze them (i.e., on the first turn or when drift is detected). We can use as_deref().unwrap_or_default() to get a read-only slice &[Tool] for verification, and only call .to_vec() to clone the tools when constructing PinnedPrefix.
| let current_tools: Vec<crate::models::Tool> = active_tools.clone().unwrap_or_default(); | |
| match &self.session.frozen_prefix { | |
| Some(frozen) => { | |
| if let Err(drift) = frozen.verify(&system_text, ¤t_tools) { | |
| tracing::debug!( | |
| target: "prefix_cache", | |
| "three-zone drift: {drift}" | |
| ); | |
| let pinned = | |
| PinnedPrefix::new(self.session.system_prompt.as_ref(), current_tools); | |
| self.session.frozen_prefix = Some(pinned.freeze()); | |
| } | |
| } | |
| None => { | |
| let pinned = | |
| PinnedPrefix::new(self.session.system_prompt.as_ref(), current_tools); | |
| self.session.frozen_prefix = Some(pinned.freeze()); | |
| } | |
| } | |
| let current_tools = active_tools.as_deref().unwrap_or_default(); | |
| match &self.session.frozen_prefix { | |
| Some(frozen) => { | |
| if let Err(drift) = frozen.verify(&system_text, current_tools) { | |
| tracing::debug!( | |
| target: "prefix_cache", | |
| "three-zone drift: {drift}" | |
| ); | |
| let pinned = | |
| PinnedPrefix::new(self.session.system_prompt.as_ref(), current_tools.to_vec()); | |
| self.session.frozen_prefix = Some(pinned.freeze()); | |
| } | |
| } | |
| None => { | |
| let pinned = | |
| PinnedPrefix::new(self.session.system_prompt.as_ref(), current_tools.to_vec()); | |
| self.session.frozen_prefix = Some(pinned.freeze()); | |
| } | |
| } |
- Comment: remove 'never auto-re-pins' (it does auto-re-freeze), describe accurately as 'auto-re-freeze on drift' - Perf: use as_deref().unwrap_or_default() to borrow &[Tool] for verify(), only to_vec() when constructing PinnedPrefix
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62a141d0b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Hey @encyc — just wanted to let you know that the FrozenPrefix Phase 2 has been harvested into the v0.8.50 triage branch (#2504). The three-zone diagnostic layer landed cleanly and all tests pass. Really tidy work — adding a second independent observation layer without any behavioral change is exactly the right approach for a phased rollout like this. Thank you! 🐋 |
Refs #2264 (Phase 2 — conservative path, warn-only).
Summary
Wires the Phase 1
FrozenPrefixtype into the request path for the firsttime. Adds a three-zone diagnostic layer alongside the existing
PrefixStabilityManager::check_and_update(). No behavioral change toexisting prefix-cache monitoring — this is an additional, independent
observation layer.
First turn: freeze baseline via
PinnedPrefix::freeze()Subsequent turns:
FrozenPrefix::verify()against current system+tools,log drift via
tracing::debug!, auto-re-freezeFiles
crates/tui/src/core/session.rscrates/tui/src/core/engine/turn_loop.rsWhat this does NOT do
PrefixStabilityManageror/cache stats/cache zoneschangesTesting
Greptile Summary
Wires the Phase 1
FrozenPrefixtype into the turn loop as a second, independent prefix-drift diagnostic layer. On the first turn it freezes a baseline; on subsequent turns it callsFrozenPrefix::verify(), logs any drift atdebuglevel, and auto-re-freezes. No user-visible events or request blocking are introduced.turn_loop.rs: Adds a ~32-line block afterPrefixStabilityManager::check_and_updatethat freezes/verifies theFrozenPrefixbaseline usingPinnedPrefix::new+freeze().session.rs: Addsfrozen_prefix: Option<FrozenPrefix>to theSessionstruct, defaulting toNone, imported fromprompt_zones.Confidence Score: 4/5
The change is warn-only with no request blocking, but it introduces a tool-hashing approach that diverges from the established PrefixStabilityManager — any tool carrying a cache_control or allowed_callers field will cause the two layers to disagree, producing false-positive drift signals that will become user-visible in Phase 3.
The tool digest function in prompt_zones uses full serde serialization while prefix_cache intentionally strips internal-only fields for the same hashing purpose. This divergence is baked in from the first freeze, making the FrozenPrefix baseline unreliable for any session with tools that have non-None internal fields. Fixing it before Phase 3 is significantly cheaper than after.
crates/tui/src/core/engine/turn_loop.rs — the verify block, and by extension prompt_zones::tool_catalog_digest which should align its serialization with prefix_cache::tool_to_api_json.
Important Files Changed
Sequence Diagram
sequenceDiagram participant TL as turn_loop participant PSM as PrefixStabilityManager participant FP as FrozenPrefix (new) participant Sess as Session TL->>PSM: check_and_update(system_text, active_tools) alt drift detected PSM-->>TL: Err(PrefixChange) TL->>TL: emit PrefixCacheChange event else stable PSM-->>TL: Ok(true) end TL->>TL: "system_text = prefix_cache::system_prompt_text(...)" TL->>TL: "current_tools = active_tools.as_deref()" alt session.frozen_prefix is None (first turn) TL->>TL: PinnedPrefix::new(system_prompt, tools.to_vec()) TL->>FP: pinned.freeze() TL->>Sess: "frozen_prefix = Some(frozen)" else session.frozen_prefix is Some(frozen) TL->>FP: frozen.verify(system_text, current_tools) alt verify Err(drift) FP-->>TL: PrefixDrift TL->>TL: tracing::debug!(drift) TL->>TL: PinnedPrefix::new(...) — auto-re-freeze TL->>Sess: "frozen_prefix = Some(new_frozen)" else verify Ok FP-->>TL: () end end TL->>TL: build MessageRequest and sendReviews (2): Last reviewed commit: "fix: clarify comment, avoid per-turn too..." | Re-trigger Greptile