From e66876249e84385fdff491d333091837669a08db Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 20 Aug 2026 15:01:43 +0900 Subject: [PATCH] fix: remove redundant system reminder wrappers --- AGENTS.md | 2 +- crates/protocol/src/lib.rs | 4 +- crates/session-store/src/system_item.rs | 81 ++++--------------- crates/tui/src/dashboard/tests.rs | 6 ++ crates/worker/src/feature/builtin/task/mod.rs | 11 +-- crates/worker/src/hook.rs | 7 +- crates/worker/src/ipc/notify_buffer.rs | 4 +- docs/design/context-history.md | 2 +- .../panel/orchestrator_idle_queue_notice.md | 2 - 9 files changed, 35 insertions(+), 84 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 07fe5362..8f186da6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,7 +14,7 @@ Workerの状態から純粋に再現可能で、且つ揮発性の無い操作 **禁止**: ターンを跨ぐことができない情報に基づいて、history に記録せずに context だけにコンテンツを差し込むこと。これをやると LLM はそれに反応して生成を行う一方、次以降のターンでhistoryに残らないため、「自分がなぜその発言/tool call をしたか」の根拠が消えるうえ、prompt cache のヒット率も低下させることになる。 -新しい input を context に乗せたいなら、必ず先に `worker.history` に append して commit すること。`history.json` への永続化はそこから自動的についてくる。Notify / WorkerEvent / `` 系はこの原則で扱う。 +新しい input を context に乗せたいなら、必ず先に `worker.history` に append して commit すること。`history.json` への永続化はそこから自動的についてくる。Notify / WorkerEvent / typed `SystemItem` reminder はこの原則で扱う。 また、キャッシュを破壊するタイミングは正確にコントロールされる必要があり、キャッシュ破壊とトークン消費のトレードオフに基づいて慎重に設計されるべきである。 --- diff --git a/crates/protocol/src/lib.rs b/crates/protocol/src/lib.rs index d44427cf..0a62b692 100644 --- a/crates/protocol/src/lib.rs +++ b/crates/protocol/src/lib.rs @@ -358,7 +358,7 @@ pub enum Event { /// `Method::Run` (kind=`UserSend`), `Method::Notify` (kind=`Notify`), /// `Method::WorkerEvent` re-injection (kind=`WorkerEvent`), and any other /// IDLE-breaking trigger. Mid-run interrupts (e.g. hook output, - /// `` injection that doesn't break IDLE) do not + /// typed system reminder insertion that doesn't break IDLE) do not /// emit `InvokeStart` — they appear as `SystemItem` only. /// /// Carries `kind` only; the payload (user text / notify message / @@ -833,7 +833,7 @@ pub enum InvokeKind { Notify, /// `Method::WorkerEvent` — typed lifecycle report from a child Worker. WorkerEvent, - /// `` etc. that crosses an IDLE boundary (mid-run + /// A typed system reminder that crosses an IDLE boundary (mid-run /// reminders that don't break IDLE are SystemItem-only and do not /// open a new Invoke). SystemReminder, diff --git a/crates/session-store/src/system_item.rs b/crates/session-store/src/system_item.rs index ed662a62..07986853 100644 --- a/crates/session-store/src/system_item.rs +++ b/crates/session-store/src/system_item.rs @@ -2,8 +2,8 @@ //! //! Items in worker history with `role:system` are never produced by the //! LLM — they are always inserted by the Worker itself (notifications, -//! file ref resolutions, child-worker lifecycle events, -//! future `` tags, …). [`SystemItem`] carries the +//! file ref resolutions, child-worker lifecycle events, reminders, …). +//! [`SystemItem`] carries the //! typed shape of each such injection so clients can dispatch on //! `kind` instead of parsing text prefixes like `[Notification] …` or //! `[File: …]`. @@ -22,10 +22,7 @@ use llm_engine::llm_client::types::Item; use protocol::WorkerEvent; use serde::{Deserialize, Serialize}; -const SYSTEM_REMINDER_OPEN: &str = ""; -const SYSTEM_REMINDER_CLOSE: &str = ""; - -/// Source policy that produced a durable `` input. +/// Source policy that produced a durable system reminder input. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum SystemReminderSource { @@ -52,57 +49,30 @@ pub struct SystemReminder { } impl SystemReminder { - /// Build a task-inactivity reminder from an unwrapped body. + /// Build a task-inactivity reminder from its plain system-message body. pub fn task_inactivity(body: impl Into) -> Self { Self::new(SystemReminderSource::TaskInactivity, body) } - /// Build a reminder from an unwrapped body. If a caller passes a body that - /// is already exactly wrapped in `` tags, normalize it back - /// to the inner body so rendering still wraps exactly once. + /// Build a reminder whose body is committed verbatim as a system message. pub fn new(source: SystemReminderSource, body: impl Into) -> Self { - let body = normalize_unwrapped_system_reminder_body(body.into()); - Self { source, body } - } - - pub fn source(&self) -> SystemReminderSource { - self.source - } - - pub fn body(&self) -> &str { - &self.body - } - - pub fn rendered_body(&self) -> String { - render_system_reminder(&self.body) + Self { + source, + body: body.into(), + } } pub fn into_system_item(self) -> SystemItem { match self.source { SystemReminderSource::TaskInactivity => SystemItem::TaskReminder { source: self.source, - body: self.rendered_body(), + body: self.body, prompt_provenance: None, }, } } } -fn normalize_unwrapped_system_reminder_body(body: String) -> String { - let trimmed = body.trim(); - if let Some(inner) = trimmed - .strip_prefix(SYSTEM_REMINDER_OPEN) - .and_then(|rest| rest.strip_suffix(SYSTEM_REMINDER_CLOSE)) - { - return inner.trim_matches('\n').to_string(); - } - body -} - -fn render_system_reminder(body: &str) -> String { - format!("{SYSTEM_REMINDER_OPEN}\n{body}\n{SYSTEM_REMINDER_CLOSE}") -} - #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct PromptRenderProvenance { #[serde(default, skip_serializing_if = "Option::is_none")] @@ -178,7 +148,7 @@ pub enum SystemItem { /// Task-management inactivity reminder inserted before an LLM request. /// `source` is the policy that produced this durable reminder; `body` is - /// the exact LLM-context text wrapped in a `` block. + /// the exact plain system-message text committed to LLM context. TaskReminder { #[serde(default = "default_task_reminder_source")] source: SystemReminderSource, @@ -324,21 +294,9 @@ mod tests { } #[test] - fn system_reminder_renders_body_once() { - let reminder = SystemReminder::task_inactivity("remember tasks"); - assert_eq!( - reminder.rendered_body(), - "\nremember tasks\n" - ); - - let already_wrapped = SystemReminder::task_inactivity( - "\nremember tasks\n", - ); - assert_eq!(already_wrapped.body(), "remember tasks"); - assert_eq!( - already_wrapped.rendered_body(), - "\nremember tasks\n" - ); + fn system_reminder_preserves_plain_body() { + let item = SystemReminder::task_inactivity("remember tasks").into_system_item(); + assert_eq!(item.history_text(), "remember tasks"); } #[test] @@ -347,10 +305,7 @@ mod tests { match item { SystemItem::TaskReminder { source, body, .. } => { assert_eq!(source, SystemReminderSource::TaskInactivity); - assert_eq!( - body, - "\nremember tasks\n" - ); + assert_eq!(body, "remember tasks"); } other => panic!("unexpected: {other:?}"), } @@ -358,10 +313,8 @@ mod tests { #[test] fn task_reminder_deserialization_defaults_legacy_source() { - let parsed: SystemItem = serde_json::from_str( - r#"{"kind":"task_reminder","body":"\nbody\n"}"#, - ) - .unwrap(); + let parsed: SystemItem = + serde_json::from_str(r#"{"kind":"task_reminder","body":"legacy body"}"#).unwrap(); match parsed { SystemItem::TaskReminder { source, .. } => { assert_eq!(source, SystemReminderSource::TaskInactivity); diff --git a/crates/tui/src/dashboard/tests.rs b/crates/tui/src/dashboard/tests.rs index 9a92ddfd..353b56ce 100644 --- a/crates/tui/src/dashboard/tests.rs +++ b/crates/tui/src/dashboard/tests.rs @@ -2980,6 +2980,12 @@ fn idle_orchestrator_gets_bounded_attention_for_new_queued_work() { .expect("idle orchestrator should receive queued-work attention"); assert_eq!(request.worker_name, "test-orchestrator"); + assert!( + request + .notice + .message + .starts_with("Workspace Dashboard observed") + ); assert!(request.notice.message.contains("00001QUEUE")); assert!(request.notice.message.contains("new_queued")); assert!(request.notice.message.contains("queued -> inprogress")); diff --git a/crates/worker/src/feature/builtin/task/mod.rs b/crates/worker/src/feature/builtin/task/mod.rs index 4cce2231..db781f9d 100644 --- a/crates/worker/src/feature/builtin/task/mod.rs +++ b/crates/worker/src/feature/builtin/task/mod.rs @@ -312,8 +312,7 @@ mod tests { let SystemItem::TaskReminder { body, .. } = &queued[0] else { panic!("unexpected system item: {:?}", queued[0]); }; - assert_eq!(body.matches("").count(), 1); - assert_eq!(body.matches("").count(), 1); + assert!(body.starts_with("Current session steps are listed below.")); assert!(body.contains("taskid 1")); assert!(body.contains("pending")); assert!(body.contains("keep going")); @@ -338,19 +337,17 @@ mod tests { panic!("unexpected system item: {:?}", queued[0]); }; assert_eq!(*source, SystemReminderSource::TaskInactivity); - assert_eq!(body.matches("").count(), 1); - assert_eq!(body.matches("").count(), 1); + assert!(body.starts_with("Current session steps are listed below.")); assert!(body.contains("typed")); } #[test] - fn render_task_reminder_body_is_unwrapped_for_system_reminder_helper() { + fn render_task_reminder_body_is_plain_system_text() { let feature = TaskFeature::new(); let task = feature.task_store().create("body".into(), String::new()); let body = render_task_reminder_body(&[task]); - assert!(!body.contains("")); - assert!(!body.contains("")); + assert!(body.starts_with("Current session steps are listed below.")); assert!(body.contains("TaskUpdate")); assert!(body.contains("taskid 1")); } diff --git a/crates/worker/src/hook.rs b/crates/worker/src/hook.rs index 5b415cd7..99be76ba 100644 --- a/crates/worker/src/hook.rs +++ b/crates/worker/src/hook.rs @@ -176,9 +176,7 @@ impl SystemItemAppendHandle { /// Queue a task-inactivity reminder for durable model-visible append. /// - /// The body should be the unwrapped reminder text; the host-side - /// `SystemReminder` renderer wraps it exactly once in `` - /// tags before commit. + /// The body is committed verbatim as the typed item's system-message text. pub fn append_task_reminder(&self, body: impl Into) { let item = SystemReminder::task_inactivity(body).into_system_item(); self.pending @@ -452,8 +450,7 @@ mod tests { assert_eq!(queued.len(), 1); match &queued[0] { SystemItem::TaskReminder { body, .. } => { - assert_eq!(body.matches("").count(), 1); - assert!(body.contains("remember tasks")); + assert_eq!(body, "remember tasks"); } other => panic!("unexpected system item: {other:?}"), } diff --git a/crates/worker/src/ipc/notify_buffer.rs b/crates/worker/src/ipc/notify_buffer.rs index 0feb47fc..0eb26c66 100644 --- a/crates/worker/src/ipc/notify_buffer.rs +++ b/crates/worker/src/ipc/notify_buffer.rs @@ -12,8 +12,8 @@ //! //! This is the **single lane** for "system messages produced by Worker //! state that should land in the next LLM request": Notify, -//! agent-visible WorkerEvent variants, and any future `` -//! injection all ride this queue. +//! agent-visible WorkerEvent variants, and any future typed system reminder +//! insertion all ride this queue. //! Per `tickets/notify-history-persist.md` and `AGENTS.md` (LLM //! context の加工原則), there is **no** "transient, history-skipping" //! lane — everything injected into a request is also committed to diff --git a/docs/design/context-history.md b/docs/design/context-history.md index ce121217..1c63af0b 100644 --- a/docs/design/context-history.md +++ b/docs/design/context-history.md @@ -24,7 +24,7 @@ Do not insert turn-crossing information directly into context without first appe Forbidden examples: - Delivering a `Notify` or `WorkerEvent` only as a temporary context note. -- Adding a `` that explains behavior but is not persisted. +- Adding a system reminder that explains behavior but is not persisted. - Rewriting old messages to include new facts. - Letting UI/controller-only state become model-visible without a committed record. diff --git a/resources/prompts/panel/orchestrator_idle_queue_notice.md b/resources/prompts/panel/orchestrator_idle_queue_notice.md index 85e6e05c..2cbf02fd 100644 --- a/resources/prompts/panel/orchestrator_idle_queue_notice.md +++ b/resources/prompts/panel/orchestrator_idle_queue_notice.md @@ -1,4 +1,3 @@ - Workspace Dashboard observed that this Orchestrator Worker is idle while queued Ticket work is present. This is bounded attention only, not scheduler authority. Do not drain the queue automatically. Verify the Ticket is still `queued`, then use the guarded `SpawnTicketCoder` operation without a separate state transition; that operation records `queued -> inprogress` only after Worker creation, initial input, assignment, and Workdir finalization are durably accepted. @@ -21,4 +20,3 @@ Additional queued Tickets omitted from this bounded notice: {{ omitted_ticket_co {% endif -%} Preserve the existing human gate, dependency/conflict/capacity/dirty-workspace checks, and duplicate-start checks using actual Ticket state, role/session claims, visible Workers, and worktrees. -