protocol拡張の実装
This commit is contained in:
parent
13c9923486
commit
b7b315cd39
3
.claude/agent-memory/ticket-reviewer/MEMORY.md
Normal file
3
.claude/agent-memory/ticket-reviewer/MEMORY.md
Normal file
|
|
@ -0,0 +1,3 @@
|
|||
- [Event broadcast pattern](project_event_broadcast_pattern.md) — Pod は event_tx: Option<broadcast::Sender<Event>> を保持、Controller が attach_notifier と同タイミングで attach
|
||||
- [Test-path omission precedent](feedback_test_path_omission.md) — 要件に挙がったテストを「共通ヘルパ経由だから省略」した場合は Approve with follow-up が相場
|
||||
- [cargo add workspace pitfall](feedback_cargo_add_workspace_pitfall.md) — ルート Cargo.toml に [workspace.dependencies] が未定義、workspace = true 指定は現状使えない
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
---
|
||||
name: cargo add workspace pitfall
|
||||
description: ルート Cargo.toml に [workspace.dependencies] が未定義なので workspace = true は使えない
|
||||
type: feedback
|
||||
---
|
||||
|
||||
ルート `Cargo.toml` は `[workspace.package]` のみを持ち `[workspace.dependencies]` を
|
||||
定義していない。したがってチケットや PR に
|
||||
`foo = { workspace = true, features = [...] }` と書かれていても、そのままでは解決しない。
|
||||
|
||||
**Why:** プロジェクトの現状流儀として、各クレートは直接バージョン指定する
|
||||
(例: `crates/session-store/Cargo.toml` の `uuid = { version = "1", features = [...] }`)。
|
||||
protocol-design (2026-04-21) レビュー時に発見。
|
||||
|
||||
**How to apply:** チケットに `workspace = true` の文言を見たら、
|
||||
- 実装が直接バージョン指定にしていれば「コードベース流儀に整合」として Follow-up 扱い、
|
||||
- `workspace = true` のまま書かれていたら「ビルドが通らないはず」として Request changes、
|
||||
- もしくは `[workspace.dependencies]` を整備する方向の提案を添える。
|
||||
|
|
@ -0,0 +1,19 @@
|
|||
---
|
||||
name: Test-path omission precedent
|
||||
description: 要件で列挙されたテストパスを「共通ヘルパなので省略」した場合の判断相場
|
||||
type: feedback
|
||||
---
|
||||
|
||||
チケット要件にテストパス (例: 成功/失敗/mid-turn の 3 本) が明示列挙されている場合、
|
||||
そのうち 1 本を「共通ヘルパ経由だから inspection で担保」として省略する実装が来たら、
|
||||
**Approve with follow-up** が相場。Blocking にはしない。
|
||||
|
||||
**Why:** 共通化されたインスツルメント (例: `send_event`) 1 点だけが共通で、
|
||||
呼び出し側の制御フロー (async 再帰・フラグ管理・エラー伝播) は個別なのが通例。
|
||||
ただしビルドと主要パスが動いており、後続チケットでテストを足すだけの差分で済むケースが多い。
|
||||
protocol-design (2026-04-21) で先例。
|
||||
|
||||
**How to apply:** 要件とテストコードを 1:1 で突き合わせ、欠けたパスがあれば
|
||||
- 制御フローが共通化されていれば Follow-up
|
||||
- 制御フローが別物 (別関数 / 別状態遷移) なら Request changes
|
||||
と切り分ける。`send_event` 型のヘルパ共通化は Follow-up 側の判断。
|
||||
|
|
@ -0,0 +1,20 @@
|
|||
---
|
||||
name: Event broadcast pattern
|
||||
description: Pod が protocol::Event を broadcast する公式パターン (Notifier と別経路)
|
||||
type: project
|
||||
---
|
||||
|
||||
Pod 内部から `protocol::Event` を broadcast する正規ルートは、`Pod` に
|
||||
`event_tx: Option<broadcast::Sender<Event>>` を持たせて `attach_event_tx` で
|
||||
Controller 側から注入する方式。`Notifier` は `Event::Notification` の
|
||||
replay バッファ専用で、他イベントは通さない。
|
||||
|
||||
**Why:** `Notifier` は Notification 型の Warn/Error レベル情報 + late subscriber への
|
||||
snapshot replay を責務にしており、Event 一般を乗せると意味が噛み合わない。
|
||||
protocol-design チケットの決定事項 6/7 で確定 (2026-04-21)。
|
||||
|
||||
**How to apply:** 新しい Pod 発の Event を追加するときは、
|
||||
1) `Pod::send_event(&self, event)` ヘルパ (`pod.rs:370-374`) を使う、
|
||||
2) Controller は `pod.attach_notifier` の直後に `pod.attach_event_tx` を呼ぶ、
|
||||
3) late subscriber への届きは期待しない (buffer 化が必要なら別チケット化)。
|
||||
Notifier 経由で新種 Event を流す PR が来たら差し戻し対象。
|
||||
270
.claude/agents/ticket-reviewer.md
Normal file
270
.claude/agents/ticket-reviewer.md
Normal file
|
|
@ -0,0 +1,270 @@
|
|||
---
|
||||
name: "ticket-reviewer"
|
||||
description: "Use this agent when a ticket implementation is submitted for review in this project (insomnia). The agent reviews the ticket's premises/requirements and the actual implementation, creates `tickets/<ticket>.review.md` with findings, and updates the original `tickets/<ticket>.md` with review status. Do NOT use this agent for general code review unrelated to a ticket. Examples:\\n<example>\\nContext: User has just finished implementing a feature described in tickets/foo.md and wants it reviewed.\\nuser: \"tickets/foo.md の実装が終わったのでレビューして\"\\nassistant: \"I'll use the Agent tool to launch the ticket-reviewer agent to review the implementation against tickets/foo.md's requirements and produce tickets/foo.review.md.\"\\n<commentary>\\nThe user is explicitly requesting a ticket-scoped review with the project's .review.md workflow, which is this agent's purpose.\\n</commentary>\\n</example>\\n<example>\\nContext: User finishes a chunk of work and mentions the ticket name.\\nuser: \"scopedfs-scripting のチケット、一通り実装出来た\"\\nassistant: \"Let me use the Agent tool to launch the ticket-reviewer agent to review the implementation and produce the review artifacts.\"\\n<commentary>\\nCompletion of a ticket implementation is the trigger for the ticket-reviewer agent per project's lifecycle (c. レビュー).\\n</commentary>\\n</example>\\n<example>\\nContext: User requests re-review after addressing feedback.\\nuser: \"指摘を反映したので再レビューお願い\"\\nassistant: \"I'll use the Agent tool to launch the ticket-reviewer agent to re-review and update the .review.md accordingly.\"\\n<commentary>\\nRe-review updates the existing .review.md and ticket status; this agent handles that workflow.\\n</commentary>\\n</example>"
|
||||
model: opus
|
||||
color: purple
|
||||
memory: project
|
||||
---
|
||||
|
||||
You are a senior reviewer specialized in the `insomnia` project. You are an expert at evaluating ticket-scoped implementations against their stated premises and requirements, and at safeguarding the codebase from unnecessary complexity or architectural drift. You operate strictly within the project's ticket lifecycle conventions defined in `CLAUDE.md`.
|
||||
|
||||
## Your Core Responsibility
|
||||
|
||||
Given a ticket (normally `tickets/<name>.md`) and its associated implementation (typically the most recent commits or working tree changes), you will:
|
||||
|
||||
1. Read the ticket thoroughly to understand its **背景・前提・要件**.
|
||||
2. Inspect the implementation (diff + surrounding code, not only the diff).
|
||||
3. Evaluate whether the ticket's requirements are fully and correctly satisfied.
|
||||
4. Evaluate architectural fit, necessity, and whether the codebase is being distorted (コードベースを歪めていないか、不必要な実装ではないか).
|
||||
5. Produce `tickets/<name>.review.md` with findings and a clear judgment.
|
||||
6. Update the original `tickets/<name>.md` to append a review status section (do NOT delete the ticket — deletion is the user's decision at completion).
|
||||
|
||||
You must NEVER run `git` write operations (commit, add, push, etc.). Git is the user's responsibility (per CLAUDE.md). You only edit/create files in the working tree.
|
||||
|
||||
## Review Methodology (in order)
|
||||
|
||||
Per the project's review policy — **architecture and ticket-requirement completion come first**:
|
||||
|
||||
### Step 1: Ticket comprehension
|
||||
- Extract 前提, 要件, 完了条件 from the ticket.
|
||||
- Note any Phase structure — but remember Phases are internal implementation order, not externally tracked progress.
|
||||
- Confirm the ticket's intended scope boundary.
|
||||
|
||||
### Step 2: Architectural & scope review (先に確認する)
|
||||
- Does the implementation respect layer boundaries? (e.g., `llm-worker` stays low-level; higher-level features live in upper layers.)
|
||||
- Are new crates named without the `insomnia-` prefix, short and consistent?
|
||||
- Were dependencies added via `cargo add` (not manual edits to Cargo.toml)?
|
||||
- Are impls split into feature modules rather than stuffed into primary files like `pod.rs`?
|
||||
- Does the implementation match stated factory/lazy-init intents where applicable?
|
||||
- Does it follow the LLM provider policy (Ollama / Codex OAuth / Anthropic API first-class; router-style common frame; no Claude OAuth reuse)?
|
||||
- Is the change the minimum necessary to satisfy the ticket, or does it over-reach?
|
||||
|
||||
### Step 3: Requirement completion check
|
||||
- Map each requirement from the ticket to concrete evidence in the diff/code.
|
||||
- Flag any requirement that is unmet, partially met, or silently deferred.
|
||||
- Verify the build-through-feature invariant: the tree must build and, unless explicitly documented as not-yet-runnable for a bounded feature, be end-to-end runnable.
|
||||
|
||||
### Step 4: Code quality & correctness
|
||||
- Investigate suspicious behavior by reading local code first (per project policy) before suspecting external causes.
|
||||
- Look for error handling, edge cases, concurrency, and resource cleanup issues.
|
||||
- Check tests: presence, meaningful coverage, and alignment with behavior.
|
||||
- Confirm naming, module organization, and API surface are consistent with existing patterns.
|
||||
|
||||
### Step 5: Judgment
|
||||
Decide one of:
|
||||
- **Approve (完了可)** — requirements met, no blocking issues.
|
||||
- **Approve with follow-up (条件付き)** — minor non-blocking items noted; user may complete or defer.
|
||||
- **Request changes (要修正)** — blocking issues must be addressed.
|
||||
|
||||
## Output Artifacts
|
||||
|
||||
### A. `tickets/<name>.review.md` (create or overwrite)
|
||||
|
||||
Use this structure (Japanese, matching project tone):
|
||||
|
||||
```markdown
|
||||
# Review: <ticket title>
|
||||
|
||||
## 前提・要件の確認
|
||||
- <要件1>: <満たされているか + 根拠>
|
||||
- <要件2>: ...
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
- <観点と判断>
|
||||
|
||||
## 指摘事項
|
||||
### Blocking
|
||||
- <項目> — <理由と該当箇所 path:line>
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
- <項目> — <理由>
|
||||
|
||||
### Nits
|
||||
- <項目>
|
||||
|
||||
## 判断
|
||||
<Approve / Approve with follow-up / Request changes> — <一文の理由>
|
||||
```
|
||||
|
||||
Omit empty sections. Cite concrete file paths and line ranges. Be concise; avoid restating obvious code.
|
||||
|
||||
### B. Update `tickets/<name>.md`
|
||||
|
||||
Append (or update if present) a trailing section like:
|
||||
|
||||
```markdown
|
||||
## Review
|
||||
- 状態: <Approve / Approve with follow-up / Request changes>
|
||||
- レビュー詳細: [./<name>.review.md](./<name>.review.md)
|
||||
- 日付: 2026-04-21
|
||||
```
|
||||
|
||||
Do not modify the ticket's 背景・要件 sections unless the user explicitly asked for it. Do not delete the ticket — deletion is reserved for the completion step (d) performed by the user.
|
||||
|
||||
## Operating Principles
|
||||
|
||||
- **Do not commit or stage anything.** File edits only. The user will handle git.
|
||||
- **Do not over-engineer the review.** Focus on whether the ticket is done and whether the codebase stays healthy.
|
||||
- **Prefer concrete citations** (path:line) over abstract complaints.
|
||||
- **Ask for clarification** only when the ticket itself is ambiguous and the ambiguity blocks judgment; otherwise make a defensible call and note it.
|
||||
- **Re-review mode**: if `.review.md` already exists, update it in place, preserving a short history of prior rounds (e.g., `## Round 2` section) so the evolution is visible until the ticket is closed.
|
||||
- **TODO.md is not your concern** unless a requirement explicitly demands it; ticket lifecycle edits to TODO.md are the user's.
|
||||
|
||||
## Quality Self-Check (before finishing)
|
||||
|
||||
1. Did I evaluate architectural fit before nitpicks?
|
||||
2. Did I map every ticket requirement to evidence?
|
||||
3. Are all blocking issues genuinely blocking (not stylistic)?
|
||||
4. Did I avoid making git writes?
|
||||
5. Did I update both `<name>.review.md` and `<name>.md`?
|
||||
6. Is my judgment line unambiguous?
|
||||
|
||||
## Agent Memory
|
||||
|
||||
**Update your agent memory** as you review tickets in this project. This builds up institutional knowledge across review sessions. Write concise notes about what you found and where.
|
||||
|
||||
Examples of what to record:
|
||||
- Recurring architectural patterns and anti-patterns observed across tickets
|
||||
- Layer boundary conventions (e.g., what belongs in llm-worker vs. upper layers) as they become clearer
|
||||
- Common requirement-miss patterns (e.g., tests omitted, build-through invariant violated)
|
||||
- Crate/module organization conventions confirmed during reviews
|
||||
- Reviewer judgment precedents — when a similar issue was Approve-with-follow-up vs. Request-changes
|
||||
- Ticket authoring patterns that correlate with smooth vs. troubled reviews
|
||||
- Project-specific policies reinforced during review (provider policy, ScopedFs scripting direction, cargo add discipline, etc.)
|
||||
|
||||
Keep entries short and link-friendly so they can be referenced in future reviews.
|
||||
|
||||
# Persistent Agent Memory
|
||||
|
||||
You have a persistent, file-based memory system at `<repo>/.claude/agent-memory/ticket-reviewer/`. This directory already exists — write to it directly with the Write tool (do not run mkdir or check for its existence).
|
||||
|
||||
You should build up this memory system over time so that future conversations can have a complete picture of who the user is, how they'd like to collaborate with you, what behaviors to avoid or repeat, and the context behind the work the user gives you.
|
||||
|
||||
If the user explicitly asks you to remember something, save it immediately as whichever type fits best. If they ask you to forget something, find and remove the relevant entry.
|
||||
|
||||
## Types of memory
|
||||
|
||||
There are several discrete types of memory that you can store in your memory system:
|
||||
|
||||
<types>
|
||||
<type>
|
||||
<name>user</name>
|
||||
<description>Contain information about the user's role, goals, responsibilities, and knowledge. Great user memories help you tailor your future behavior to the user's preferences and perspective. Your goal in reading and writing these memories is to build up an understanding of who the user is and how you can be most helpful to them specifically. For example, you should collaborate with a senior software engineer differently than a student who is coding for the very first time. Keep in mind, that the aim here is to be helpful to the user. Avoid writing memories about the user that could be viewed as a negative judgement or that are not relevant to the work you're trying to accomplish together.</description>
|
||||
<when_to_save>When you learn any details about the user's role, preferences, responsibilities, or knowledge</when_to_save>
|
||||
<how_to_use>When your work should be informed by the user's profile or perspective. For example, if the user is asking you to explain a part of the code, you should answer that question in a way that is tailored to the specific details that they will find most valuable or that helps them build their mental model in relation to domain knowledge they already have.</how_to_use>
|
||||
<examples>
|
||||
user: I'm a data scientist investigating what logging we have in place
|
||||
assistant: [saves user memory: user is a data scientist, currently focused on observability/logging]
|
||||
|
||||
user: I've been writing Go for ten years but this is my first time touching the React side of this repo
|
||||
assistant: [saves user memory: deep Go expertise, new to React and this project's frontend — frame frontend explanations in terms of backend analogues]
|
||||
</examples>
|
||||
</type>
|
||||
<type>
|
||||
<name>feedback</name>
|
||||
<description>Guidance the user has given you about how to approach work — both what to avoid and what to keep doing. These are a very important type of memory to read and write as they allow you to remain coherent and responsive to the way you should approach work in the project. Record from failure AND success: if you only save corrections, you will avoid past mistakes but drift away from approaches the user has already validated, and may grow overly cautious.</description>
|
||||
<when_to_save>Any time the user corrects your approach ("no not that", "don't", "stop doing X") OR confirms a non-obvious approach worked ("yes exactly", "perfect, keep doing that", accepting an unusual choice without pushback). Corrections are easy to notice; confirmations are quieter — watch for them. In both cases, save what is applicable to future conversations, especially if surprising or not obvious from the code. Include *why* so you can judge edge cases later.</when_to_save>
|
||||
<how_to_use>Let these memories guide your behavior so that the user does not need to offer the same guidance twice.</how_to_use>
|
||||
<body_structure>Lead with the rule itself, then a **Why:** line (the reason the user gave — often a past incident or strong preference) and a **How to apply:** line (when/where this guidance kicks in). Knowing *why* lets you judge edge cases instead of blindly following the rule.</body_structure>
|
||||
<examples>
|
||||
user: don't mock the database in these tests — we got burned last quarter when mocked tests passed but the prod migration failed
|
||||
assistant: [saves feedback memory: integration tests must hit a real database, not mocks. Reason: prior incident where mock/prod divergence masked a broken migration]
|
||||
|
||||
user: stop summarizing what you just did at the end of every response, I can read the diff
|
||||
assistant: [saves feedback memory: this user wants terse responses with no trailing summaries]
|
||||
|
||||
user: yeah the single bundled PR was the right call here, splitting this one would've just been churn
|
||||
assistant: [saves feedback memory: for refactors in this area, user prefers one bundled PR over many small ones. Confirmed after I chose this approach — a validated judgment call, not a correction]
|
||||
</examples>
|
||||
</type>
|
||||
<type>
|
||||
<name>project</name>
|
||||
<description>Information that you learn about ongoing work, goals, initiatives, bugs, or incidents within the project that is not otherwise derivable from the code or git history. Project memories help you understand the broader context and motivation behind the work the user is doing within this working directory.</description>
|
||||
<when_to_save>When you learn who is doing what, why, or by when. These states change relatively quickly so try to keep your understanding of this up to date. Always convert relative dates in user messages to absolute dates when saving (e.g., "Thursday" → "2026-03-05"), so the memory remains interpretable after time passes.</when_to_save>
|
||||
<how_to_use>Use these memories to more fully understand the details and nuance behind the user's request and make better informed suggestions.</how_to_use>
|
||||
<body_structure>Lead with the fact or decision, then a **Why:** line (the motivation — often a constraint, deadline, or stakeholder ask) and a **How to apply:** line (how this should shape your suggestions). Project memories decay fast, so the why helps future-you judge whether the memory is still load-bearing.</body_structure>
|
||||
<examples>
|
||||
user: we're freezing all non-critical merges after Thursday — mobile team is cutting a release branch
|
||||
assistant: [saves project memory: merge freeze begins 2026-03-05 for mobile release cut. Flag any non-critical PR work scheduled after that date]
|
||||
|
||||
user: the reason we're ripping out the old auth middleware is that legal flagged it for storing session tokens in a way that doesn't meet the new compliance requirements
|
||||
assistant: [saves project memory: auth middleware rewrite is driven by legal/compliance requirements around session token storage, not tech-debt cleanup — scope decisions should favor compliance over ergonomics]
|
||||
</examples>
|
||||
</type>
|
||||
<type>
|
||||
<name>reference</name>
|
||||
<description>Stores pointers to where information can be found in external systems. These memories allow you to remember where to look to find up-to-date information outside of the project directory.</description>
|
||||
<when_to_save>When you learn about resources in external systems and their purpose. For example, that bugs are tracked in a specific project in Linear or that feedback can be found in a specific Slack channel.</when_to_save>
|
||||
<how_to_use>When the user references an external system or information that may be in an external system.</how_to_use>
|
||||
<examples>
|
||||
user: check the Linear project "INGEST" if you want context on these tickets, that's where we track all pipeline bugs
|
||||
assistant: [saves reference memory: pipeline bugs are tracked in Linear project "INGEST"]
|
||||
|
||||
user: the Grafana board at grafana.internal/d/api-latency is what oncall watches — if you're touching request handling, that's the thing that'll page someone
|
||||
assistant: [saves reference memory: grafana.internal/d/api-latency is the oncall latency dashboard — check it when editing request-path code]
|
||||
</examples>
|
||||
</type>
|
||||
</types>
|
||||
|
||||
## What NOT to save in memory
|
||||
|
||||
- Code patterns, conventions, architecture, file paths, or project structure — these can be derived by reading the current project state.
|
||||
- Git history, recent changes, or who-changed-what — `git log` / `git blame` are authoritative.
|
||||
- Debugging solutions or fix recipes — the fix is in the code; the commit message has the context.
|
||||
- Anything already documented in CLAUDE.md files.
|
||||
- Ephemeral task details: in-progress work, temporary state, current conversation context.
|
||||
|
||||
These exclusions apply even when the user explicitly asks you to save. If they ask you to save a PR list or activity summary, ask what was *surprising* or *non-obvious* about it — that is the part worth keeping.
|
||||
|
||||
## How to save memories
|
||||
|
||||
Saving a memory is a two-step process:
|
||||
|
||||
**Step 1** — write the memory to its own file (e.g., `user_role.md`, `feedback_testing.md`) using this frontmatter format:
|
||||
|
||||
```markdown
|
||||
---
|
||||
name: {{memory name}}
|
||||
description: {{one-line description — used to decide relevance in future conversations, so be specific}}
|
||||
type: {{user, feedback, project, reference}}
|
||||
---
|
||||
|
||||
{{memory content — for feedback/project types, structure as: rule/fact, then **Why:** and **How to apply:** lines}}
|
||||
```
|
||||
|
||||
**Step 2** — add a pointer to that file in `MEMORY.md`. `MEMORY.md` is an index, not a memory — each entry should be one line, under ~150 characters: `- [Title](file.md) — one-line hook`. It has no frontmatter. Never write memory content directly into `MEMORY.md`.
|
||||
|
||||
- `MEMORY.md` is always loaded into your conversation context — lines after 200 will be truncated, so keep the index concise
|
||||
- Keep the name, description, and type fields in memory files up-to-date with the content
|
||||
- Organize memory semantically by topic, not chronologically
|
||||
- Update or remove memories that turn out to be wrong or outdated
|
||||
- Do not write duplicate memories. First check if there is an existing memory you can update before writing a new one.
|
||||
|
||||
## When to access memories
|
||||
- When memories seem relevant, or the user references prior-conversation work.
|
||||
- You MUST access memory when the user explicitly asks you to check, recall, or remember.
|
||||
- If the user says to *ignore* or *not use* memory: Do not apply remembered facts, cite, compare against, or mention memory content.
|
||||
- Memory records can become stale over time. Use memory as context for what was true at a given point in time. Before answering the user or building assumptions based solely on information in memory records, verify that the memory is still correct and up-to-date by reading the current state of the files or resources. If a recalled memory conflicts with current information, trust what you observe now — and update or remove the stale memory rather than acting on it.
|
||||
|
||||
## Before recommending from memory
|
||||
|
||||
A memory that names a specific function, file, or flag is a claim that it existed *when the memory was written*. It may have been renamed, removed, or never merged. Before recommending it:
|
||||
|
||||
- If the memory names a file path: check the file exists.
|
||||
- If the memory names a function or flag: grep for it.
|
||||
- If the user is about to act on your recommendation (not just asking about history), verify first.
|
||||
|
||||
"The memory says X exists" is not the same as "X exists now."
|
||||
|
||||
A memory that summarizes repo state (activity logs, architecture snapshots) is frozen in time. If the user asks about *recent* or *current* state, prefer `git log` or reading the code over recalling the snapshot.
|
||||
|
||||
## Memory and other forms of persistence
|
||||
Memory is one of several persistence mechanisms available to you as you assist the user in a given conversation. The distinction is often that memory can be recalled in future conversations and should not be used for persisting information that is only useful within the scope of the current conversation.
|
||||
- When to use or update a plan instead of memory: If you are about to start a non-trivial implementation task and would like to reach alignment with the user on your approach you should use a Plan rather than saving this information to memory. Similarly, if you already have a plan within the conversation and you have changed your approach persist that change by updating the plan rather than saving a memory.
|
||||
- When to use or update tasks instead of memory: When you need to break your work in current conversation into discrete steps or keep track of your progress use tasks instead of saving to memory. Tasks are great for persisting information about the work that needs to be done in the current conversation, but memory should be reserved for information that will be useful in future conversations.
|
||||
|
||||
- Since this memory is project-scope and shared with your team via version control, tailor your memories to this project
|
||||
|
||||
## MEMORY.md
|
||||
|
||||
Your MEMORY.md is currently empty. When you save new memories, they will appear here.
|
||||
5
Cargo.lock
generated
5
Cargo.lock
generated
|
|
@ -2177,6 +2177,7 @@ dependencies = [
|
|||
"serde",
|
||||
"serde_json",
|
||||
"tokio",
|
||||
"uuid",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
|
|
@ -3623,9 +3624,9 @@ checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821"
|
|||
|
||||
[[package]]
|
||||
name = "uuid"
|
||||
version = "1.23.0"
|
||||
version = "1.23.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "5ac8b6f42ead25368cf5b098aeb3dc8a1a2c05a3eee8a9a1a68c640edbfc79d9"
|
||||
checksum = "ddd74a9687298c6858e9b88ec8935ec45d22e8fd5e6394fa1bd4e99a87789c76"
|
||||
dependencies = [
|
||||
"atomic",
|
||||
"getrandom 0.4.2",
|
||||
|
|
|
|||
|
|
@ -102,6 +102,9 @@ impl PodController {
|
|||
// AGENTS.md ingestion during the first turn) can emit user-facing
|
||||
// notifications on the same channel.
|
||||
pod.attach_notifier(notifier.clone());
|
||||
// Also hand the raw broadcast sender so Pod-internal operations
|
||||
// can emit typed lifecycle `Event`s (currently: compact progress).
|
||||
pod.attach_event_tx(event_tx.clone());
|
||||
|
||||
// Start socket server (lives as a background task, cleaned up on drop via RuntimeDir)
|
||||
let _socket_server = SocketServer::start(&handle).await?;
|
||||
|
|
|
|||
|
|
@ -27,7 +27,8 @@ use crate::runtime_dir;
|
|||
use crate::scope_lock::{self, ScopeAllocationGuard, ScopeLockError};
|
||||
use crate::system_prompt::{SystemPromptContext, SystemPromptError, SystemPromptTemplate};
|
||||
use crate::usage_tracker::UsageTracker;
|
||||
use protocol::{NotificationLevel, NotificationSource};
|
||||
use protocol::{Event, NotificationLevel, NotificationSource};
|
||||
use tokio::sync::broadcast;
|
||||
use async_trait::async_trait;
|
||||
use llm_worker::interceptor::PreRequestAction;
|
||||
|
||||
|
|
@ -120,6 +121,11 @@ pub struct Pod<C: LlmClient, St: Store> {
|
|||
/// User-facing notification sink attached by the Controller at
|
||||
/// spawn time. `None` in tests / direct `Pod::new` usage.
|
||||
notifier: Option<Notifier>,
|
||||
/// Broadcast sender for typed lifecycle `Event`s (compact progress,
|
||||
/// etc.). Attached by the Controller alongside `notifier`. Unlike
|
||||
/// notifications, events sent here are NOT replayed to clients that
|
||||
/// connect after the fact — they are fire-and-forget broadcasts.
|
||||
event_tx: Option<broadcast::Sender<Event>>,
|
||||
/// Queue of pending `Method::Notify` notifications awaiting
|
||||
/// injection into the next LLM request. Shared with the
|
||||
/// PodInterceptor installed in `ensure_interceptor_installed`.
|
||||
|
|
@ -176,6 +182,7 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
tracker: None,
|
||||
system_prompt_template: None,
|
||||
notifier: None,
|
||||
event_tx: None,
|
||||
pending_notifications: NotificationBuffer::new(),
|
||||
scope_allocation: None,
|
||||
callback_socket: None,
|
||||
|
|
@ -241,6 +248,7 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
tracker: None,
|
||||
system_prompt_template: None,
|
||||
notifier: None,
|
||||
event_tx: None,
|
||||
pending_notifications: NotificationBuffer::new(),
|
||||
scope_allocation: None,
|
||||
callback_socket: None,
|
||||
|
|
@ -343,12 +351,30 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
self.notifier = Some(notifier);
|
||||
}
|
||||
|
||||
/// Attach the broadcast sender used for typed lifecycle `Event`s.
|
||||
///
|
||||
/// The Controller wires this alongside [`attach_notifier`] so that
|
||||
/// Pod-internal operations (currently: compaction) can surface
|
||||
/// progress to connected clients.
|
||||
pub fn attach_event_tx(&mut self, event_tx: broadcast::Sender<Event>) {
|
||||
self.event_tx = Some(event_tx);
|
||||
}
|
||||
|
||||
fn notify(&self, level: NotificationLevel, source: NotificationSource, message: String) {
|
||||
if let Some(n) = self.notifier.as_ref() {
|
||||
n.notify(level, source, message);
|
||||
}
|
||||
}
|
||||
|
||||
/// Broadcast a typed `Event` to connected clients. No-op when no
|
||||
/// `event_tx` is attached (tests / direct `Pod::new` usage) or when
|
||||
/// no clients are currently subscribed.
|
||||
fn send_event(&self, event: Event) {
|
||||
if let Some(tx) = self.event_tx.as_ref() {
|
||||
let _ = tx.send(event);
|
||||
}
|
||||
}
|
||||
|
||||
/// Push a `Method::Notify` entry onto the pending buffer.
|
||||
///
|
||||
/// The notification will be injected as an `Item::system_message`
|
||||
|
|
@ -682,12 +708,14 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
.map(|s| s.retained_tokens())
|
||||
.unwrap_or(manifest::defaults::COMPACT_RETAINED_TOKENS);
|
||||
|
||||
self.send_event(Event::CompactStart);
|
||||
match self.compact(retained).await {
|
||||
Ok(new_session_id) => {
|
||||
info!(
|
||||
new_session_id = %new_session_id,
|
||||
"Compaction succeeded, resuming execution"
|
||||
);
|
||||
self.send_event(Event::CompactDone { new_session_id });
|
||||
if let Some(ref state) = self.compact_state {
|
||||
state.record_compact_success();
|
||||
}
|
||||
|
|
@ -695,6 +723,9 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
}
|
||||
Err(e) => {
|
||||
warn!(error = %e, "Compaction failed during run");
|
||||
self.send_event(Event::CompactFailed {
|
||||
error: e.to_string(),
|
||||
});
|
||||
self.notify(
|
||||
NotificationLevel::Error,
|
||||
NotificationSource::Compactor,
|
||||
|
|
@ -723,17 +754,22 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
}
|
||||
|
||||
let retained = state.retained_tokens();
|
||||
self.send_event(Event::CompactStart);
|
||||
match self.compact(retained).await {
|
||||
Ok(new_session_id) => {
|
||||
info!(
|
||||
new_session_id = %new_session_id,
|
||||
"Proactive post-run compaction succeeded"
|
||||
);
|
||||
self.send_event(Event::CompactDone { new_session_id });
|
||||
state.record_compact_success();
|
||||
Ok(())
|
||||
}
|
||||
Err(e) => {
|
||||
warn!(error = %e, "Proactive post-run compaction failed");
|
||||
self.send_event(Event::CompactFailed {
|
||||
error: e.to_string(),
|
||||
});
|
||||
self.notify(
|
||||
NotificationLevel::Warn,
|
||||
NotificationSource::Compactor,
|
||||
|
|
@ -1143,6 +1179,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
tracker: None,
|
||||
system_prompt_template,
|
||||
notifier: None,
|
||||
event_tx: None,
|
||||
pending_notifications: NotificationBuffer::new(),
|
||||
scope_allocation: Some(scope_allocation),
|
||||
callback_socket: None,
|
||||
|
|
@ -1202,6 +1239,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
tracker: None,
|
||||
system_prompt_template,
|
||||
notifier: None,
|
||||
event_tx: None,
|
||||
pending_notifications: NotificationBuffer::new(),
|
||||
scope_allocation: Some(scope_allocation),
|
||||
callback_socket: Some(callback_socket),
|
||||
|
|
|
|||
318
crates/pod/tests/compact_events_test.rs
Normal file
318
crates/pod/tests/compact_events_test.rs
Normal file
|
|
@ -0,0 +1,318 @@
|
|||
//! Compact lifecycle `Event` broadcasting.
|
||||
//!
|
||||
//! Covers three paths:
|
||||
//! - `try_post_run_compact` success → `CompactStart + CompactDone`
|
||||
//! - `try_post_run_compact` failure → `CompactStart + CompactFailed`
|
||||
//! - mid-turn `do_compact_and_resume` success → `CompactStart + CompactDone`
|
||||
//! (driven by `compact_request_threshold` → `PreRequestAction::Yield`)
|
||||
|
||||
use std::pin::Pin;
|
||||
use std::sync::Arc;
|
||||
use std::sync::atomic::{AtomicUsize, Ordering};
|
||||
|
||||
use async_trait::async_trait;
|
||||
use futures::Stream;
|
||||
use llm_worker::Worker;
|
||||
use llm_worker::llm_client::event::{Event as LlmEvent, ResponseStatus, StatusEvent};
|
||||
use llm_worker::llm_client::{ClientError, LlmClient, Request};
|
||||
use protocol::Event;
|
||||
use session_store::FsStore;
|
||||
use tokio::sync::broadcast;
|
||||
|
||||
use pod::Pod;
|
||||
|
||||
#[derive(Clone)]
|
||||
struct MockClient {
|
||||
responses: Arc<Vec<Vec<LlmEvent>>>,
|
||||
call_count: Arc<AtomicUsize>,
|
||||
}
|
||||
|
||||
impl MockClient {
|
||||
fn new(responses: Vec<Vec<LlmEvent>>) -> Self {
|
||||
Self {
|
||||
responses: Arc::new(responses),
|
||||
call_count: Arc::new(AtomicUsize::new(0)),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl LlmClient for MockClient {
|
||||
fn clone_boxed(&self) -> Box<dyn LlmClient> {
|
||||
Box::new(self.clone())
|
||||
}
|
||||
|
||||
async fn stream(
|
||||
&self,
|
||||
_request: Request,
|
||||
) -> Result<Pin<Box<dyn Stream<Item = Result<LlmEvent, ClientError>> + Send>>, ClientError>
|
||||
{
|
||||
let count = self.call_count.fetch_add(1, Ordering::SeqCst);
|
||||
if count >= self.responses.len() {
|
||||
return Err(ClientError::Config("mock client exhausted".into()));
|
||||
}
|
||||
let events = self.responses[count].clone();
|
||||
let stream = futures::stream::iter(events.into_iter().map(Ok));
|
||||
Ok(Box::pin(stream))
|
||||
}
|
||||
}
|
||||
|
||||
fn single_text_events(text: &str) -> Vec<LlmEvent> {
|
||||
vec![
|
||||
LlmEvent::text_block_start(0),
|
||||
LlmEvent::text_delta(0, text),
|
||||
LlmEvent::text_block_stop(0, None),
|
||||
LlmEvent::Status(StatusEvent {
|
||||
status: ResponseStatus::Completed,
|
||||
}),
|
||||
]
|
||||
}
|
||||
|
||||
/// `single_text_events` + a UsageEvent so the Pod's `usage_history`
|
||||
/// picks up a measurement, which is how `pre_llm_request` decides
|
||||
/// whether to yield mid-turn.
|
||||
fn text_events_with_usage(text: &str, input_tokens: u64) -> Vec<LlmEvent> {
|
||||
vec![
|
||||
LlmEvent::text_block_start(0),
|
||||
LlmEvent::text_delta(0, text),
|
||||
LlmEvent::text_block_stop(0, None),
|
||||
LlmEvent::usage(input_tokens, 1),
|
||||
LlmEvent::Status(StatusEvent {
|
||||
status: ResponseStatus::Completed,
|
||||
}),
|
||||
]
|
||||
}
|
||||
|
||||
fn write_summary_tool_use_events(call_id: &str, text: &str) -> Vec<LlmEvent> {
|
||||
let input = serde_json::json!({ "text": text }).to_string();
|
||||
vec![
|
||||
LlmEvent::tool_use_start(0, call_id, "write_summary"),
|
||||
LlmEvent::tool_input_delta(0, input),
|
||||
LlmEvent::tool_use_stop(0),
|
||||
LlmEvent::Status(StatusEvent {
|
||||
status: ResponseStatus::Completed,
|
||||
}),
|
||||
]
|
||||
}
|
||||
|
||||
// A low compact_threshold guarantees `try_post_run_compact` will fire
|
||||
// the first time we check after a run.
|
||||
const POST_RUN_MANIFEST_TOML: &str = r#"
|
||||
[pod]
|
||||
name = "test-pod"
|
||||
pwd = "./"
|
||||
|
||||
[model]
|
||||
scheme = "anthropic"
|
||||
model_id = "test-model"
|
||||
|
||||
[worker]
|
||||
max_tokens = 100
|
||||
|
||||
[compaction]
|
||||
compact_threshold = 1
|
||||
compact_retained_tokens = 0
|
||||
|
||||
[[scope.allow]]
|
||||
target = "./"
|
||||
permission = "write"
|
||||
"#;
|
||||
|
||||
// `compact_request_threshold` drives the PodInterceptor's mid-turn yield
|
||||
// path. `compact_threshold` is left unset so the post-run check stays inert.
|
||||
const MID_TURN_MANIFEST_TOML: &str = r#"
|
||||
[pod]
|
||||
name = "test-pod"
|
||||
pwd = "./"
|
||||
|
||||
[model]
|
||||
scheme = "anthropic"
|
||||
model_id = "test-model"
|
||||
|
||||
[worker]
|
||||
max_tokens = 100
|
||||
|
||||
[compaction]
|
||||
compact_request_threshold = 100
|
||||
compact_retained_tokens = 0
|
||||
|
||||
[[scope.allow]]
|
||||
target = "./"
|
||||
permission = "write"
|
||||
"#;
|
||||
|
||||
async fn make_pod_with_manifest(
|
||||
manifest_toml: &str,
|
||||
client: MockClient,
|
||||
) -> Pod<MockClient, FsStore> {
|
||||
let manifest = pod::PodManifest::from_toml(manifest_toml).unwrap();
|
||||
|
||||
let store_tmp = tempfile::tempdir().unwrap();
|
||||
let store = FsStore::new(store_tmp.path()).await.unwrap();
|
||||
std::mem::forget(store_tmp);
|
||||
|
||||
let pwd_tmp = tempfile::tempdir().unwrap();
|
||||
let pwd = pwd_tmp.path().to_path_buf();
|
||||
let scope = pod::Scope::writable(&pwd).unwrap();
|
||||
std::mem::forget(pwd_tmp);
|
||||
|
||||
let worker = Worker::new(client);
|
||||
Pod::new(manifest, worker, store, pwd, scope).await.unwrap()
|
||||
}
|
||||
|
||||
async fn make_pod(client: MockClient) -> Pod<MockClient, FsStore> {
|
||||
make_pod_with_manifest(POST_RUN_MANIFEST_TOML, client).await
|
||||
}
|
||||
|
||||
/// Drain whatever events are already queued on `rx`. Non-blocking.
|
||||
fn drain(rx: &mut broadcast::Receiver<Event>) -> Vec<Event> {
|
||||
let mut out = Vec::new();
|
||||
loop {
|
||||
match rx.try_recv() {
|
||||
Ok(ev) => out.push(ev),
|
||||
Err(_) => break,
|
||||
}
|
||||
}
|
||||
out
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn post_run_compact_success_broadcasts_start_and_done() {
|
||||
// Responses: (1) first run returns short text, (2) compact worker
|
||||
// emits write_summary then closes (two LLM calls inside the compact
|
||||
// worker: one for write_summary, one that the compact loop consumes
|
||||
// as the final "I'm done" close response).
|
||||
let client = MockClient::new(vec![
|
||||
single_text_events("hi"),
|
||||
write_summary_tool_use_events("call-1", "summary"),
|
||||
single_text_events("done"),
|
||||
]);
|
||||
let mut pod = make_pod(client).await;
|
||||
|
||||
let (tx, mut rx) = broadcast::channel::<Event>(64);
|
||||
pod.attach_event_tx(tx);
|
||||
|
||||
pod.run("first").await.unwrap();
|
||||
// Drain run events so only compact events remain in `rx`.
|
||||
let _ = drain(&mut rx);
|
||||
|
||||
pod.try_post_run_compact().await.unwrap();
|
||||
|
||||
let events = drain(&mut rx);
|
||||
let kinds: Vec<&str> = events
|
||||
.iter()
|
||||
.map(|e| match e {
|
||||
Event::CompactStart => "start",
|
||||
Event::CompactDone { .. } => "done",
|
||||
Event::CompactFailed { .. } => "failed",
|
||||
_ => "other",
|
||||
})
|
||||
.collect();
|
||||
assert!(
|
||||
kinds.contains(&"start") && kinds.contains(&"done"),
|
||||
"expected CompactStart + CompactDone in {kinds:?}"
|
||||
);
|
||||
assert!(
|
||||
!kinds.contains(&"failed"),
|
||||
"unexpected CompactFailed in {kinds:?}"
|
||||
);
|
||||
|
||||
// CompactDone carries the new session id.
|
||||
let new_id_in_event = events.iter().find_map(|e| match e {
|
||||
Event::CompactDone { new_session_id } => Some(*new_session_id),
|
||||
_ => None,
|
||||
});
|
||||
assert!(new_id_in_event.is_some(), "CompactDone missing");
|
||||
assert_eq!(new_id_in_event.unwrap(), pod.session_id());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn mid_turn_compact_success_broadcasts_start_and_done() {
|
||||
// Path: `do_compact_and_resume` via PreRequestAction::Yield.
|
||||
//
|
||||
// Sequence of LLM calls the mock will serve:
|
||||
// [0] first run completes with a UsageEvent(1000 > threshold=100) so
|
||||
// the next run's pre_llm_request will yield.
|
||||
// [1] compact worker emits `write_summary` tool call.
|
||||
// [2] compact worker closes (its final "done" response).
|
||||
// [3] resume() after compact makes one more LLM call.
|
||||
let client = MockClient::new(vec![
|
||||
text_events_with_usage("a", 1000),
|
||||
write_summary_tool_use_events("call-1", "summary"),
|
||||
single_text_events("done"),
|
||||
single_text_events("b"),
|
||||
]);
|
||||
let mut pod = make_pod_with_manifest(MID_TURN_MANIFEST_TOML, client).await;
|
||||
|
||||
let (tx, mut rx) = broadcast::channel::<Event>(64);
|
||||
pod.attach_event_tx(tx);
|
||||
|
||||
// First run populates usage_history above the request threshold.
|
||||
pod.run("first").await.unwrap();
|
||||
let _ = drain(&mut rx);
|
||||
|
||||
// Second run: pre_llm_request yields immediately, Worker returns
|
||||
// Yielded, handle_worker_result routes into do_compact_and_resume.
|
||||
pod.run("second").await.unwrap();
|
||||
|
||||
let events = drain(&mut rx);
|
||||
let kinds: Vec<&str> = events
|
||||
.iter()
|
||||
.map(|e| match e {
|
||||
Event::CompactStart => "start",
|
||||
Event::CompactDone { .. } => "done",
|
||||
Event::CompactFailed { .. } => "failed",
|
||||
_ => "other",
|
||||
})
|
||||
.collect();
|
||||
assert!(
|
||||
kinds.contains(&"start") && kinds.contains(&"done"),
|
||||
"expected CompactStart + CompactDone in {kinds:?}"
|
||||
);
|
||||
assert!(
|
||||
!kinds.contains(&"failed"),
|
||||
"unexpected CompactFailed in {kinds:?}"
|
||||
);
|
||||
|
||||
let new_id_in_event = events.iter().find_map(|e| match e {
|
||||
Event::CompactDone { new_session_id } => Some(*new_session_id),
|
||||
_ => None,
|
||||
});
|
||||
assert_eq!(new_id_in_event, Some(pod.session_id()));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn post_run_compact_failure_broadcasts_start_and_failed() {
|
||||
// Only the first run has a response. Compaction will run the
|
||||
// compact worker which immediately exhausts the mock → failure.
|
||||
let client = MockClient::new(vec![single_text_events("hi")]);
|
||||
let mut pod = make_pod(client).await;
|
||||
|
||||
let (tx, mut rx) = broadcast::channel::<Event>(64);
|
||||
pod.attach_event_tx(tx);
|
||||
|
||||
pod.run("first").await.unwrap();
|
||||
let _ = drain(&mut rx);
|
||||
|
||||
// Best-effort: returns Ok(()) even on failure, but emits CompactFailed.
|
||||
pod.try_post_run_compact().await.unwrap();
|
||||
|
||||
let events = drain(&mut rx);
|
||||
let kinds: Vec<&str> = events
|
||||
.iter()
|
||||
.map(|e| match e {
|
||||
Event::CompactStart => "start",
|
||||
Event::CompactDone { .. } => "done",
|
||||
Event::CompactFailed { .. } => "failed",
|
||||
_ => "other",
|
||||
})
|
||||
.collect();
|
||||
assert!(
|
||||
kinds.contains(&"start") && kinds.contains(&"failed"),
|
||||
"expected CompactStart + CompactFailed in {kinds:?}"
|
||||
);
|
||||
assert!(
|
||||
!kinds.contains(&"done"),
|
||||
"unexpected CompactDone in {kinds:?}"
|
||||
);
|
||||
}
|
||||
|
|
@ -8,3 +8,4 @@ license.workspace = true
|
|||
serde = { version = "1.0", features = ["derive"] }
|
||||
serde_json = "1.0"
|
||||
tokio = { version = "1.51.1", features = ["io-util"] }
|
||||
uuid = { version = "1.23.1", features = ["serde"] }
|
||||
|
|
|
|||
|
|
@ -130,6 +130,23 @@ pub enum Event {
|
|||
greeting: Greeting,
|
||||
},
|
||||
Notification(Notification),
|
||||
/// Pod has started compacting the current session.
|
||||
///
|
||||
/// Fired immediately before a compaction run. Success is signalled by
|
||||
/// `CompactDone` (with the new `SessionId`); failure by `CompactFailed`.
|
||||
/// Broadcast to all clients; not replayed to late subscribers.
|
||||
CompactStart,
|
||||
/// Compaction completed and the session was rotated.
|
||||
///
|
||||
/// `new_session_id` is the UUID of the freshly created session that
|
||||
/// replaced the old history.
|
||||
CompactDone {
|
||||
new_session_id: uuid::Uuid,
|
||||
},
|
||||
/// Compaction failed. The session is unchanged.
|
||||
CompactFailed {
|
||||
error: String,
|
||||
},
|
||||
Shutdown,
|
||||
}
|
||||
|
||||
|
|
@ -442,6 +459,49 @@ mod tests {
|
|||
assert_eq!(parsed["data"]["timestamp_ms"], 1_700_000_000_000i64);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn event_compact_start_roundtrip() {
|
||||
let event = Event::CompactStart;
|
||||
let json = serde_json::to_string(&event).unwrap();
|
||||
assert_eq!(json, r#"{"event":"compact_start"}"#);
|
||||
let decoded: Event = serde_json::from_str(&json).unwrap();
|
||||
assert!(matches!(decoded, Event::CompactStart));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn event_compact_done_roundtrip() {
|
||||
let id = uuid::Uuid::parse_str("0192f0e8-4d84-7d6e-a000-000000000001").unwrap();
|
||||
let event = Event::CompactDone { new_session_id: id };
|
||||
let json = serde_json::to_string(&event).unwrap();
|
||||
let parsed: serde_json::Value = serde_json::from_str(&json).unwrap();
|
||||
assert_eq!(parsed["event"], "compact_done");
|
||||
assert_eq!(
|
||||
parsed["data"]["new_session_id"],
|
||||
"0192f0e8-4d84-7d6e-a000-000000000001"
|
||||
);
|
||||
let decoded: Event = serde_json::from_str(&json).unwrap();
|
||||
match decoded {
|
||||
Event::CompactDone { new_session_id } => assert_eq!(new_session_id, id),
|
||||
other => panic!("expected CompactDone, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn event_compact_failed_roundtrip() {
|
||||
let event = Event::CompactFailed {
|
||||
error: "provider 429".into(),
|
||||
};
|
||||
let json = serde_json::to_string(&event).unwrap();
|
||||
let parsed: serde_json::Value = serde_json::from_str(&json).unwrap();
|
||||
assert_eq!(parsed["event"], "compact_failed");
|
||||
assert_eq!(parsed["data"]["error"], "provider 429");
|
||||
let decoded: Event = serde_json::from_str(&json).unwrap();
|
||||
match decoded {
|
||||
Event::CompactFailed { error } => assert_eq!(error, "provider 429"),
|
||||
other => panic!("expected CompactFailed, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn event_error_format() {
|
||||
let event = Event::Error {
|
||||
|
|
|
|||
|
|
@ -189,6 +189,29 @@ impl App {
|
|||
self.current_tool = None;
|
||||
}
|
||||
Event::ToolCallArgsDelta { .. } => {}
|
||||
Event::CompactStart => {
|
||||
self.output_queue.push(OutputItem::Padded(
|
||||
MessageKind::NoticeWarn,
|
||||
"[compact] starting".to_string(),
|
||||
));
|
||||
}
|
||||
Event::CompactDone { new_session_id } => {
|
||||
let short = new_session_id
|
||||
.to_string()
|
||||
.chars()
|
||||
.take(8)
|
||||
.collect::<String>();
|
||||
self.output_queue.push(OutputItem::Padded(
|
||||
MessageKind::NoticeWarn,
|
||||
format!("[compact] done (new session {short})"),
|
||||
));
|
||||
}
|
||||
Event::CompactFailed { error } => {
|
||||
self.output_queue.push(OutputItem::Padded(
|
||||
MessageKind::NoticeError,
|
||||
format!("[compact error] {error}"),
|
||||
));
|
||||
}
|
||||
Event::Notification(notification) => {
|
||||
let kind = match notification.level {
|
||||
NotificationLevel::Warn => MessageKind::NoticeWarn,
|
||||
|
|
|
|||
|
|
@ -84,6 +84,28 @@ Event::CompactFailed { error: String }
|
|||
- 汎用 `SessionChanged` は作らない（YAGNI — fork 等が実装される時まで判断を保留）。
|
||||
- TUI 側は現状 session_id を利用していないので、イベントを受け取るだけでよい（将来 GetHistory 再取得などに使う）。
|
||||
|
||||
### 5. wire 型 — `protocol` が `uuid::Uuid` を直接扱う
|
||||
|
||||
- `SessionId` のパースは `protocol` クレートの責務。`protocol/Cargo.toml` に
|
||||
`uuid = { workspace = true, features = ["serde"] }` を追加し、
|
||||
`Event::CompactDone { new_session_id: uuid::Uuid }` として型付けする。
|
||||
- `session-store::SessionId` は `uuid::Uuid` のエイリアスなので、Pod 側は変換なしで渡せる。
|
||||
- 文字列経由にはしない（wire を弱く型付けして嬉しいことがない）。
|
||||
|
||||
### 6. broadcast 手段 — Pod が `event_tx` を直接保持
|
||||
|
||||
- `Notifier` は `Event::Notification` の replay バッファ専用のまま残し、compact 系イベントは
|
||||
通さない（意味が噛み合わない）。
|
||||
- `Pod` に `event_tx: Option<broadcast::Sender<Event>>` を持たせ、
|
||||
`attach_notifier` と同じタイミングで Controller 側から渡す。
|
||||
- compact 発火点では `self.event_tx.as_ref().map(|tx| tx.send(...))` で直接流す。
|
||||
|
||||
### 7. late subscriber への再配信 — しない
|
||||
|
||||
- compact イベントはバッファせず、broadcast 時点で購読していないクライアントには届かない。
|
||||
- TUI は現状 session_id を使っていないため、接続後に直近の compact を知る必要がない。
|
||||
- 必要になった時点（fork や複数クライアント同時接続が現実になった時）で別チケットで buffer 化。
|
||||
|
||||
### 4. Permission ask/reply — 別チケットで実装
|
||||
|
||||
- permission-extension-point の段階 3 で追加する。
|
||||
|
|
@ -94,12 +116,20 @@ Event::CompactFailed { error: String }
|
|||
|
||||
## 本チケットで実装するもの
|
||||
|
||||
1. `Event::CompactStart` / `Event::CompactDone { new_session_id: SessionId }` / `Event::CompactFailed { error: String }` を追加する。
|
||||
- `SessionId` の wire 表現は既存 `session_store::SessionId` をそのまま `Serialize` できれば使う。外部型に依存するなら `String` 化する。
|
||||
2. `crates/pod/src/pod.rs` の compact 発火 2 箇所で対応する Event を broadcast する。
|
||||
3. `crates/tui/src/app.rs` の `handle_pod_event` に 3 分岐を追加し、最低限の表示を行う（アイコン + メッセージ、詳細 UI は別チケット）。
|
||||
4. テスト:
|
||||
- Event の JSON roundtrip（`protocol` クレート）
|
||||
1. `protocol` クレートに `uuid = { workspace = true, features = ["serde"] }` を追加し、
|
||||
`Event::CompactStart` / `Event::CompactDone { new_session_id: uuid::Uuid }` /
|
||||
`Event::CompactFailed { error: String }` を追加する。
|
||||
2. `Pod` に `event_tx: Option<broadcast::Sender<Event>>` を持たせ、Controller 側から
|
||||
`Notifier` と同じタイミングで渡す。
|
||||
3. `crates/pod/src/pod.rs` の compact 発火 2 箇所（`do_compact_and_resume` /
|
||||
`try_post_run_compact`）で start / 成功（CompactDone） / 失敗（CompactFailed）を broadcast する。
|
||||
4. `crates/tui/src/app.rs` の `handle_pod_event` に 3 分岐を追加し、
|
||||
既存の `NoticeWarn` / `NoticeError` と同じ枠で最低限のテキストを表示する。
|
||||
- `[compact] starting`
|
||||
- `[compact] done (new session <uuid先頭8字>)`
|
||||
- `[compact error] <message>`
|
||||
5. テスト:
|
||||
- Event の JSON roundtrip（`protocol` クレート）: 3 バリアント + uuid の shape
|
||||
- 成功/失敗/mid-turn の各パスで Event が発行されることを確認する統合テスト（`pod` クレート）
|
||||
|
||||
## 本チケットで実装しないもの
|
||||
|
|
@ -114,3 +144,9 @@ Event::CompactFailed { error: String }
|
|||
- Event の肥大化が気になってきたら、`Event::Stream(StreamEvent)` のようにカテゴリ分けしてネストする余地はある。ただし現状 15 バリアントで破綻していないので先送り。
|
||||
- Protocol のバージョニングは未着手。クライアントの互換性問題が顕在化した時点で `Event::Hello { protocol_version }` のような握手を追加する。
|
||||
- Broadcast channel の特性上、遅い client では drop が起き得る。現状はログのみだが、compact 等の重要イベントを落とすとまずいので将来は per-client queue への置き換えを検討する。
|
||||
|
||||
## Review
|
||||
|
||||
- 状態: Approve with follow-up
|
||||
- レビュー詳細: [./protocol-design.review.md](./protocol-design.review.md)
|
||||
- 日付: 2026-04-21
|
||||
|
|
|
|||
58
tickets/protocol-design.review.md
Normal file
58
tickets/protocol-design.review.md
Normal file
|
|
@ -0,0 +1,58 @@
|
|||
# Review: Protocol の設計
|
||||
|
||||
対象コミット: working tree (未コミット)
|
||||
レビュー日: 2026-04-21
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
チケット「本チケットで実装するもの」1〜5 を実装と照合した。
|
||||
|
||||
1. **`protocol` クレートに `uuid` 追加 + 3 バリアント追加**
|
||||
- `crates/protocol/Cargo.toml:11` に `uuid = { version = "1.23.1", features = ["serde"] }` を追加済み。
|
||||
- `crates/protocol/src/lib.rs:138-149` に `Event::CompactStart` / `Event::CompactDone { new_session_id: uuid::Uuid }` / `Event::CompactFailed { error: String }` を追加。決定事項 5 の型方針（`uuid::Uuid` 直接）に一致。
|
||||
- ただしチケット本文は `uuid = { workspace = true, features = ["serde"] }` と書いているが、ルート `Cargo.toml` には `[workspace.dependencies]` が存在せず、そのままでは解決不能。既存の `session-store` と同じく直接バージョン指定にしているのは実装として合理的（コードベース流儀に整合）。チケット文言との齟齬は残っているので、どちらかを合わせるのが望ましい（後述）。
|
||||
2. **`Pod` が `event_tx` を保持 / Controller が `Notifier` と同タイミングで渡す**
|
||||
- `crates/pod/src/pod.rs:124-128` にフィールド追加、`attach_event_tx` は `pod.rs:358-361`。全 4 箇所のコンストラクタで `event_tx: None` 初期化済み（`pod.rs:185, 251, 1182, 1242`）。
|
||||
- `crates/pod/src/controller.rs:104-107` で `attach_notifier` の直後に `attach_event_tx(event_tx.clone())`。決定事項 6 通り。
|
||||
3. **compact 発火 2 箇所で start/成功/失敗を broadcast**
|
||||
- `do_compact_and_resume`: `pod.rs:711` (start) / `pod.rs:718` (done) / `pod.rs:726-728` (failed)。
|
||||
- `try_post_run_compact`: `pod.rs:757` (start) / `pod.rs:764` (done) / `pod.rs:770-772` (failed)。
|
||||
- 既存 `notify(...)` の失敗通知（Compactor Warn/Error）も残っており、決定事項 2 の「初期移行では重複して出しても害はない」に整合。
|
||||
4. **TUI `handle_pod_event` の 3 分岐**
|
||||
- `crates/tui/src/app.rs:192-214` で 3 バリアントを `NoticeWarn`/`NoticeError` 枠に追加。
|
||||
- 表示文言もチケット指定通り（`[compact] starting` / `[compact] done (new session <先頭8字>)` / `[compact error] <message>`）。
|
||||
5. **テスト**
|
||||
- protocol クレート: JSON roundtrip 3 本（`event_compact_start_roundtrip` / `event_compact_done_roundtrip` / `event_compact_failed_roundtrip`）— `new_session_id` の shape（ハイフン付き UUID 文字列）も `serde_json::Value` 経由で検証済み。
|
||||
- pod クレート統合テスト: `crates/pod/tests/compact_events_test.rs` に `post_run_compact_success_broadcasts_start_and_done` / `post_run_compact_failure_broadcasts_start_and_failed` の 2 本。
|
||||
- **mid-turn (`do_compact_and_resume`) のテストは欠落**。ファイル先頭コメントで「`send_event` ヘルパが共通のため inspection で担保」と明記しているが、チケット要件 5 は「成功/失敗/mid-turn の各パスで Event が発行されることを確認する統合テスト」と明示的に 3 パスを要求している。要件に対して実装側の判断で削ったことになるので Follow-up 以上。
|
||||
|
||||
`cargo build -p protocol -p pod -p tui` と `cargo test -p protocol --lib` / `cargo test -p pod --test compact_events_test` はローカルで通過。ビルドが通り機能が実行可能であるという全体不変条件は保たれている。
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
- **層分離**: `protocol` は wire 型に専念し、Pod 層は `broadcast::Sender<Event>` を保持して直接 push。`Notifier` は `Event::Notification` の replay 専用として残り、compact 系は通していない。決定事項 6/7 通りで、Notifier の責務を汚さない良い切り分け。
|
||||
- **send_event の書き方**: `pod.rs:370-374` の `fn send_event(&self, event: Event)` は `if let Some(tx) = self.event_tx.as_ref() { let _ = tx.send(event); }`。subscriber 不在時の `SendError` を潰す挙動（決定事項 7 の「late subscriber には届かない」）と一致。late subscriber への再配信をしないのも合意通り。
|
||||
- **依存追加規律**: `uuid` は `cargo add -p protocol uuid --features serde` で追加された形跡（バージョン番号 `1.23.1` が精密指定で入っている）。`Cargo.lock` の patch-level 更新も自然。`cargo add` 利用ポリシーを満たす。
|
||||
- **クレート命名・モジュール分割**: 新規クレート追加なし。`Pod` 本体への追加は `attach_event_tx` / `send_event` の 2 メソッド + 1 フィールドに留め、compact ロジックは既存関数内の差し込みに限定。機能別ファイル分離のポリシーを破っていない（今回は既存メソッド内への数行挿入なので妥当）。
|
||||
- **プロトコルの健全性**: wire path は `broadcast::Sender<Event>` → `socket_server::handle_connection` の `rx.recv()` → `writer.write(&event)` で既存と同じルート。新バリアントを足すだけで TUI まで透過的に届く（`crates/pod/src/socket_server.rs:83-91`）。
|
||||
- **代替案の妥当性**: `Notifier` に compact を相乗りさせる案は、「Notification は Warn/Error レベル付き replay バッファで意味が噛み合わない」ため避けた判断は正しい。`Notifier` は今後も compact 系を通さないことを前提にできるようになった。
|
||||
- **YAGNI 方針**: `SessionChanged` の一般化や `request_id` フィールドの先取り導入を見送り、compact に絞った最小変更。コードベースを歪めていない。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Blocking
|
||||
- なし。
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
- **mid-turn のテスト省略はチケット要件 5 に対する未達**（`crates/pod/tests/compact_events_test.rs:1-6` のファイル冒頭で省略理由を明記済み）。`do_compact_and_resume` は `just_compacted` フラグと `Box::pin` の async 再帰、`resume()` 呼び出しが挟まるため、`try_post_run_compact` とは制御フローが別物。共通化しているのは `send_event` のみで、発火点の有無・順序・失敗時の `notify + Err(e)` 伝播は別のテスト対象である。後続チケットに流してよい程度の軽い欠落だが、要件の文言通り 1 本（例: 成功時 + `PodError::CompactThrash` での挙動、または Yielded からの compact success）追加するのが望ましい。
|
||||
- **ticket 文言と Cargo.toml の表現齟齬**: ticket では `uuid = { workspace = true, ... }` と書いているのに、実装は直接バージョン指定（`session-store` と同じ書き方）。どちらが正解かはプロジェクト方針だが、ワークスペースに `[workspace.dependencies]` が未定義なので直接指定の方が現状整合する。チケットの文言を「`session-store` と同じく直接バージョン指定で追加する」に修正する、あるいは `[workspace.dependencies]` を用意して両方揃える、のどちらか。後続対応で構わない。
|
||||
- **CompactThrash 時は Start/Failed のいずれも発火しない**（`pod.rs:698-702`）。thrash 検出は compact を実行前に弾くので `CompactStart` が出ないのは正しいが、TUI 側は「compact が必要だったのに thrash で諦めた」ことを知る手段がない。現状は `PodError::CompactThrash` が `run` の Result として返り、その先で誰かがエラー Event 化する想定と思われるが、今回のスコープでは扱わなくてよい。将来 compact 周りの可視化を強化するときに再考を。
|
||||
- **`CompactDone` の短縮 UUID 表現** (`tui/src/app.rs:199-203`): `to_string().chars().take(8)` は UTF-8 上 ASCII なので実質 `&uuid_str[..8]` と等価。実害はないが、`new_session_id.as_simple().to_string()[..8].to_string()` か `format!("{new_session_id:.8}")` 相当にするとアロケーション 1 回で済む。nit に近い。
|
||||
|
||||
### Nits
|
||||
- `crates/protocol/src/lib.rs:138-149` の docstring が `CompactStart`/`CompactDone`/`CompactFailed` で個別に説明されていて読みやすい。`CompactStart` のコメントに「Broadcast to all clients; not replayed to late subscribers.」まで書いているので、決定事項 7 が wire-type 側に滲み出して次のレビューアの手助けになる。このレベルで継続されると良い。
|
||||
- `compact_events_test.rs:121-131` の `drain` ヘルパは `loop { match ... { Err(_) => break } }` で lagged/closed を区別していない。このテストでは channel 容量 64 で十分なので問題ないが、将来テストを増やす際には `try_recv` の `TryRecvError::Lagged(n)` を拾って assert するとより堅牢。
|
||||
|
||||
## 判断
|
||||
|
||||
**Approve with follow-up** — 要件 1〜4 と アーキテクチャ方針（決定事項 5/6/7）は全て達成。ビルド・テストも通過。要件 5 のうち mid-turn テストの省略がチケット文言に対してわずかに未達だが、インスツルメント自体は 1 箇所共通 (`send_event`) で呼び出し側の差分も小さいため blocker ではなく follow-up として処理可能。チケット側の `uuid` 依存表記（`workspace = true`）と実装の直接指定の齟齬も合わせて、どちらかを揃える後続対応を推奨する。
|
||||
Loading…
Reference in New Issue
Block a user