Compare commits
3 Commits
fa84d48c62
...
8ed739261f
| Author | SHA1 | Date | |
|---|---|---|---|
| 8ed739261f | |||
| 5a29c90786 | |||
| 0d66b397af |
10
Cargo.lock
generated
10
Cargo.lock
generated
|
|
@ -82,6 +82,15 @@ version = "1.0.102"
|
|||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c"
|
||||
|
||||
[[package]]
|
||||
name = "arc-swap"
|
||||
version = "1.9.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "6a3a1fd6f75306b68087b831f025c712524bcb19aad54e557b1129cfa0a2b207"
|
||||
dependencies = [
|
||||
"rustversion",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "assert-json-diff"
|
||||
version = "2.0.2"
|
||||
|
|
@ -1696,6 +1705,7 @@ dependencies = [
|
|||
name = "manifest"
|
||||
version = "0.1.0"
|
||||
dependencies = [
|
||||
"arc-swap",
|
||||
"llm-worker",
|
||||
"protocol",
|
||||
"serde",
|
||||
|
|
|
|||
7
TODO.md
7
TODO.md
|
|
@ -2,26 +2,19 @@
|
|||
- [ ] Workflow 実装 → [tickets/workflow.md](tickets/workflow.md)
|
||||
- [ ] 内部 Worker / 内部 Pod の Workflow 化 → [tickets/internal-worker-workflow.md](tickets/internal-worker-workflow.md)
|
||||
- [ ] Agent Skills を Workflow として ingest → [tickets/agent-skills.md](tickets/agent-skills.md)
|
||||
- [ ] ツール設計
|
||||
- [ ] Bash ツール (Permission 層と統合) → [tickets/bash-tool.md](tickets/bash-tool.md)
|
||||
- [ ] パーミッション: パターンベースのツール実行制御 → [tickets/permission-extension-point.md](tickets/permission-extension-point.md)
|
||||
- [ ] Pod CLI: マニフェスト関連フラグの整理 → [tickets/pod-cli-manifest-flags.md](tickets/pod-cli-manifest-flags.md)
|
||||
- [ ] OpenAI Responses: sampling パラメータの取り扱い → [tickets/responses-sampling-params.md](tickets/responses-sampling-params.md)
|
||||
- [ ] llm-worker のエラー耐性
|
||||
- [ ] HTTP transient リトライ → [tickets/llm-worker-transient-retry.md](tickets/llm-worker-transient-retry.md)
|
||||
- [ ] ストリーム途中失敗時の継続 → [tickets/llm-worker-stream-continuation.md](tickets/llm-worker-stream-continuation.md)
|
||||
- [ ] Pod オーケストレーション
|
||||
- [ ] 動的 Scope 変更 → [tickets/dynamic-scope.md](tickets/dynamic-scope.md)
|
||||
- [ ] ネイティブ GUI クライアント MVP → [tickets/native-gui-mvp.md](tickets/native-gui-mvp.md)
|
||||
- [ ] TUI 拡充
|
||||
- [ ] フルスクリーン化によるオーバーホール → [tickets/tui-fullscreen-overhaul.md](tickets/tui-fullscreen-overhaul.md)
|
||||
- [ ] Run 中の入力キューイング → [tickets/tui-input-queue.md](tickets/tui-input-queue.md)
|
||||
- [ ] ユーザーマニフェストのモデル設定 wizard → [tickets/tui-user-model-setup.md](tickets/tui-user-model-setup.md)
|
||||
- [ ] マウスホイールでのヒストリースクロール → [tickets/tui-mouse-scroll.md](tickets/tui-mouse-scroll.md)
|
||||
- [ ] サブミット入力
|
||||
- [ ] TUI 補完 + 型付き atom 化 → [tickets/submit-tui-completion.md](tickets/submit-tui-completion.md)
|
||||
- [ ] FileRef リゾルバ → [tickets/submit-file-ref-resolver.md](tickets/submit-file-ref-resolver.md)
|
||||
- [ ] メモリ機構
|
||||
- [ ] Phase 2 consolidation + 整理 → [tickets/memory-phase2-consolidation.md](tickets/memory-phase2-consolidation.md)
|
||||
- [ ] 使用頻度メトリクス + Knowledge 化候補レポート → [tickets/memory-usage-metrics.md](tickets/memory-usage-metrics.md)
|
||||
- ワークスペースのメモリーをLintするヘッドレスCLI
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ edition.workspace = true
|
|||
license.workspace = true
|
||||
|
||||
[dependencies]
|
||||
arc-swap = "1"
|
||||
llm-worker = { workspace = true }
|
||||
protocol = { workspace = true }
|
||||
serde = { workspace = true, features = ["derive"] }
|
||||
|
|
|
|||
|
|
@ -15,7 +15,7 @@ pub use model::{
|
|||
};
|
||||
pub use paths::user_manifest_path;
|
||||
pub use protocol::{Permission, ScopeRule};
|
||||
pub use scope::{Scope, ScopeError};
|
||||
pub use scope::{Scope, ScopeError, SharedScope};
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::num::NonZeroU32;
|
||||
|
|
|
|||
|
|
@ -8,6 +8,9 @@
|
|||
|
||||
use std::ffi::OsString;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::sync::{Arc, Mutex};
|
||||
|
||||
use arc_swap::{ArcSwap, Guard};
|
||||
|
||||
use crate::{Permission, ScopeConfig, ScopeRule};
|
||||
|
||||
|
|
@ -182,6 +185,38 @@ impl Scope {
|
|||
.map(|r| r.target.as_path())
|
||||
}
|
||||
|
||||
/// Build a new [`Scope`] equal to `self` with `extra_allow` appended
|
||||
/// to the allow set. Used by dynamic-scope grow paths
|
||||
/// (e.g. controller adding the bash-output Read rule, future
|
||||
/// external `GrantScope`).
|
||||
pub fn with_added_allow_rules(
|
||||
&self,
|
||||
extra_allow: impl IntoIterator<Item = ScopeRule>,
|
||||
) -> Result<Self, ScopeError> {
|
||||
let mut config = ScopeConfig {
|
||||
allow: self.allow_rules(),
|
||||
deny: self.deny_rules(),
|
||||
};
|
||||
config.allow.extend(extra_allow);
|
||||
Self::from_config(&config)
|
||||
}
|
||||
|
||||
/// Build a new [`Scope`] equal to `self` with `extra_deny` appended
|
||||
/// to the deny set. Used by dynamic-scope shrink paths
|
||||
/// (e.g. SpawnPod-style delegation that strips Write from the
|
||||
/// spawner without touching its allow rules).
|
||||
pub fn with_added_deny_rules(
|
||||
&self,
|
||||
extra_deny: impl IntoIterator<Item = ScopeRule>,
|
||||
) -> Result<Self, ScopeError> {
|
||||
let mut config = ScopeConfig {
|
||||
allow: self.allow_rules(),
|
||||
deny: self.deny_rules(),
|
||||
};
|
||||
config.deny.extend(extra_deny);
|
||||
Self::from_config(&config)
|
||||
}
|
||||
|
||||
/// Human-readable grouping of allow rules, suitable for embedding in
|
||||
/// LLM system prompts. Deny rules are intentionally omitted — they
|
||||
/// only cap effective permission and surface them would mislead the
|
||||
|
|
@ -230,6 +265,71 @@ impl Scope {
|
|||
}
|
||||
}
|
||||
|
||||
/// Shared, atomically-swappable view of a [`Scope`].
|
||||
///
|
||||
/// Built around [`ArcSwap`] so the hot path (permission checks inside
|
||||
/// `ScopedFs`) reads the current scope lock-free. Mutators are
|
||||
/// serialised by an internal `Mutex` so concurrent `update` calls do
|
||||
/// not lose each other's contributions.
|
||||
///
|
||||
/// All clones share the same underlying state — a `SharedScope` cloned
|
||||
/// out to multiple consumers (Pod, ScopedFs, future grant/revoke
|
||||
/// callers) sees every update.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct SharedScope {
|
||||
inner: Arc<SharedScopeInner>,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
struct SharedScopeInner {
|
||||
scope: ArcSwap<Scope>,
|
||||
/// Serialises read-modify-write update transactions so a producer
|
||||
/// can read the current scope, build a derived one, and store it
|
||||
/// without losing concurrent updates.
|
||||
write_lock: Mutex<()>,
|
||||
}
|
||||
|
||||
impl SharedScope {
|
||||
/// Wrap an owned [`Scope`] in a shared, atomically-swappable handle.
|
||||
pub fn new(scope: Scope) -> Self {
|
||||
Self {
|
||||
inner: Arc::new(SharedScopeInner {
|
||||
scope: ArcSwap::from_pointee(scope),
|
||||
write_lock: Mutex::new(()),
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
/// Snapshot the current scope. Cheap and lock-free; the returned
|
||||
/// guard borrows the live scope for as long as it is held.
|
||||
pub fn load(&self) -> Guard<Arc<Scope>> {
|
||||
self.inner.scope.load()
|
||||
}
|
||||
|
||||
/// Snapshot the current scope into an owned `Arc<Scope>`. Useful
|
||||
/// when the caller needs a value that outlives the load guard
|
||||
/// (e.g. cloning into another struct).
|
||||
pub fn snapshot(&self) -> Arc<Scope> {
|
||||
self.inner.scope.load_full()
|
||||
}
|
||||
|
||||
/// Read-modify-write transaction. `f` is called with the current
|
||||
/// scope and returns a derived one (or an error). The internal
|
||||
/// write lock ensures that two concurrent `update` calls see each
|
||||
/// other's results — the second observes the first's output as its
|
||||
/// input.
|
||||
pub fn update<F>(&self, f: F) -> Result<(), ScopeError>
|
||||
where
|
||||
F: FnOnce(&Scope) -> Result<Scope, ScopeError>,
|
||||
{
|
||||
let _guard = self.inner.write_lock.lock().expect("scope mutex poisoned");
|
||||
let current = self.inner.scope.load();
|
||||
let new = f(¤t)?;
|
||||
self.inner.scope.store(Arc::new(new));
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
impl ResolvedRule {
|
||||
fn matches(&self, path: &Path) -> bool {
|
||||
if self.recursive {
|
||||
|
|
@ -545,4 +645,87 @@ mod tests {
|
|||
let deep = dir.path().join("a/b/c/new.txt");
|
||||
assert!(scope.is_writable(&deep));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn with_added_allow_rules_grows_readable_set() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let extra = TempDir::new().unwrap();
|
||||
let base = Scope::writable(dir.path()).unwrap();
|
||||
assert!(!base.is_readable(&extra.path().join("x")));
|
||||
let extended = base
|
||||
.with_added_allow_rules([ScopeRule {
|
||||
target: extra.path().to_path_buf(),
|
||||
permission: Permission::Read,
|
||||
recursive: true,
|
||||
}])
|
||||
.unwrap();
|
||||
assert!(extended.is_readable(&extra.path().join("x")));
|
||||
assert!(extended.is_writable(&dir.path().join("y")));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn with_added_deny_rules_demotes_write_to_read() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let sub = dir.path().join("sub");
|
||||
std::fs::create_dir(&sub).unwrap();
|
||||
let base = Scope::writable(dir.path()).unwrap();
|
||||
let demoted = base
|
||||
.with_added_deny_rules([ScopeRule {
|
||||
target: sub.clone(),
|
||||
permission: Permission::Write,
|
||||
recursive: true,
|
||||
}])
|
||||
.unwrap();
|
||||
let f = sub.join("a.txt");
|
||||
assert_eq!(demoted.permission_at(&f), Some(Permission::Read));
|
||||
assert_eq!(
|
||||
demoted.permission_at(&dir.path().join("top.txt")),
|
||||
Some(Permission::Write)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shared_scope_load_returns_current_value() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let shared = SharedScope::new(Scope::writable(dir.path()).unwrap());
|
||||
assert!(shared.load().is_writable(&dir.path().join("a.txt")));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shared_scope_update_replaces_view_atomically() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let sub = dir.path().join("sub");
|
||||
std::fs::create_dir(&sub).unwrap();
|
||||
let shared = SharedScope::new(Scope::writable(dir.path()).unwrap());
|
||||
let target = sub.join("a.txt");
|
||||
assert_eq!(shared.load().permission_at(&target), Some(Permission::Write));
|
||||
shared
|
||||
.update(|cur| {
|
||||
cur.with_added_deny_rules([ScopeRule {
|
||||
target: sub.clone(),
|
||||
permission: Permission::Write,
|
||||
recursive: true,
|
||||
}])
|
||||
})
|
||||
.unwrap();
|
||||
assert_eq!(shared.load().permission_at(&target), Some(Permission::Read));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shared_scope_clones_share_state() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let extra = TempDir::new().unwrap();
|
||||
let a = SharedScope::new(Scope::writable(dir.path()).unwrap());
|
||||
let b = a.clone();
|
||||
assert!(!b.load().is_readable(&extra.path().join("x")));
|
||||
a.update(|cur| {
|
||||
cur.with_added_allow_rules([ScopeRule {
|
||||
target: extra.path().to_path_buf(),
|
||||
permission: Permission::Read,
|
||||
recursive: true,
|
||||
}])
|
||||
})
|
||||
.unwrap();
|
||||
assert!(b.load().is_readable(&extra.path().join("x")));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -83,7 +83,7 @@ impl PodController {
|
|||
|
||||
// Snapshot pod-immutable values needed for tool factories so the
|
||||
// mutable worker borrow below doesn't conflict with reads on `pod`.
|
||||
let scope_for_tools = pod.scope().clone();
|
||||
let scope_handle = pod.scope().clone();
|
||||
let pwd_for_tools = pod.pwd().to_path_buf();
|
||||
let spawner_name = pod.manifest().pod.name.clone();
|
||||
let spawner_model = pod.manifest().model.clone();
|
||||
|
|
@ -108,6 +108,28 @@ impl PodController {
|
|||
// can emit typed lifecycle `Event`s (currently: compact progress).
|
||||
pod.attach_event_tx(event_tx.clone());
|
||||
|
||||
// Bash spills long outputs to a per-pod subdir under the runtime
|
||||
// dir. Push a recursive `allow(Read)` for that path into the
|
||||
// Pod's runtime scope so the agent can `Read` saved files
|
||||
// without polluting the workspace. The Pod's SharedScope is the
|
||||
// single source of truth — every ScopedFs (builtin tools,
|
||||
// fs_view, compact worker) reads from it, and any future scope
|
||||
// mutation (SpawnPod-style revoke, future GrantScope)
|
||||
// propagates through it.
|
||||
let bash_output_dir = runtime_dir.path().join("bash-output");
|
||||
std::fs::create_dir_all(&bash_output_dir).map_err(|e| {
|
||||
std::io::Error::other(format!(
|
||||
"create bash output dir {}: {e}",
|
||||
bash_output_dir.display()
|
||||
))
|
||||
})?;
|
||||
pod.add_scope_rules([manifest::ScopeRule {
|
||||
target: bash_output_dir.clone(),
|
||||
permission: manifest::Permission::Read,
|
||||
recursive: true,
|
||||
}])
|
||||
.map_err(std::io::Error::other)?;
|
||||
|
||||
// Stashed during tool registration below so we can attach a
|
||||
// `PodFsView` to the shared state once the latter exists.
|
||||
let fs_for_view: tools::ScopedFs;
|
||||
|
|
@ -229,31 +251,13 @@ impl PodController {
|
|||
// context compaction) can ask which files the agent has been
|
||||
// touching.
|
||||
//
|
||||
// Bash spills long outputs to a per-pod subdir under the
|
||||
// runtime dir. We layer a recursive `allow(Read)` rule for
|
||||
// that path on top of the user-facing scope so the agent can
|
||||
// `Read` the saved files without polluting the workspace.
|
||||
// Same approach memory takes for its deny rules: round-trip
|
||||
// through `ScopeConfig` and rebuild via `from_config`.
|
||||
let bash_output_dir = runtime_dir.path().join("bash-output");
|
||||
std::fs::create_dir_all(&bash_output_dir).map_err(|e| {
|
||||
std::io::Error::other(format!(
|
||||
"create bash output dir {}: {e}",
|
||||
bash_output_dir.display()
|
||||
))
|
||||
})?;
|
||||
let mut scope_config = manifest::ScopeConfig {
|
||||
allow: scope_for_tools.allow_rules(),
|
||||
deny: scope_for_tools.deny_rules(),
|
||||
};
|
||||
scope_config.allow.push(manifest::ScopeRule {
|
||||
target: bash_output_dir.clone(),
|
||||
permission: manifest::Permission::Read,
|
||||
recursive: true,
|
||||
});
|
||||
let scope_with_bash = manifest::Scope::from_config(&scope_config)
|
||||
.map_err(std::io::Error::other)?;
|
||||
let fs = tools::ScopedFs::new(scope_with_bash, pwd_for_tools.clone());
|
||||
// The Pod's SharedScope (already augmented with the
|
||||
// bash-output Read rule above) is the single source of
|
||||
// truth — every ScopedFs (builtin tools, fs_view, compact
|
||||
// worker) reads from it, and any future scope mutation
|
||||
// (SpawnPod-style revoke, future GrantScope) propagates
|
||||
// through it.
|
||||
let fs = tools::ScopedFs::with_shared_scope(scope_handle.clone(), pwd_for_tools.clone());
|
||||
let tracker = tools::Tracker::new();
|
||||
// The same ScopedFs also powers the IPC `ListCompletions`
|
||||
// query — keep a clone for the FS view we attach below,
|
||||
|
|
@ -292,6 +296,7 @@ impl PodController {
|
|||
spawned_registry.clone(),
|
||||
self_parent_socket.clone(),
|
||||
spawner_model.clone(),
|
||||
scope_handle.clone(),
|
||||
));
|
||||
worker.register_tool(send_to_pod_tool(spawned_registry.clone()));
|
||||
worker.register_tool(read_pod_output_tool(spawned_registry.clone()));
|
||||
|
|
@ -873,7 +878,7 @@ where
|
|||
cwd: pod.pwd().display().to_string(),
|
||||
provider: provider_name,
|
||||
model: model_id,
|
||||
scope_summary: pod.scope().summary(),
|
||||
scope_summary: pod.scope_snapshot().summary(),
|
||||
tools: tool_names,
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -10,7 +10,10 @@ use llm_worker::{ToolOutputLimits, UsageRecord, Worker, WorkerError, WorkerResul
|
|||
use session_store::{EntryHash, SessionId, SessionStartState, Store, StoreError};
|
||||
use tracing::{info, warn};
|
||||
|
||||
use manifest::{PodManifest, PodManifestConfig, ResolveError, Scope, ScopeError, WorkerManifest};
|
||||
use manifest::{
|
||||
PodManifest, PodManifestConfig, ResolveError, Scope, ScopeError, ScopeRule, SharedScope,
|
||||
WorkerManifest,
|
||||
};
|
||||
|
||||
use crate::compact::state::CompactState;
|
||||
use crate::compact::usage_tracker::UsageTracker;
|
||||
|
|
@ -60,8 +63,11 @@ pub struct Pod<C: LlmClient, St: Store> {
|
|||
head_hash: Option<EntryHash>,
|
||||
/// Absolute working directory of the Pod.
|
||||
pwd: PathBuf,
|
||||
/// Resolved scope — always present.
|
||||
scope: Scope,
|
||||
/// Shared, atomically-swappable view of the Pod's resolved scope.
|
||||
/// Cloned out to `ScopedFs` instances (builtin tools, fs_view,
|
||||
/// compact worker) so scope updates propagate to every consumer
|
||||
/// at the next permission check.
|
||||
scope: SharedScope,
|
||||
hook_builder: HookRegistryBuilder,
|
||||
interceptor_installed: bool,
|
||||
/// Shared compaction state (present when compact_threshold is configured).
|
||||
|
|
@ -185,7 +191,7 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
session_id,
|
||||
head_hash: None,
|
||||
pwd,
|
||||
scope,
|
||||
scope: SharedScope::new(scope),
|
||||
hook_builder: HookRegistryBuilder::new(),
|
||||
interceptor_installed: false,
|
||||
compact_state: None,
|
||||
|
|
@ -252,11 +258,46 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
&self.pwd
|
||||
}
|
||||
|
||||
/// The Pod's directory scope.
|
||||
pub fn scope(&self) -> &Scope {
|
||||
/// The Pod's directory scope, as a shared atomically-swappable
|
||||
/// handle. Clone it to share scope state with another consumer
|
||||
/// (e.g. a tool that needs to mutate scope dynamically).
|
||||
pub fn scope(&self) -> &SharedScope {
|
||||
&self.scope
|
||||
}
|
||||
|
||||
/// Snapshot the current scope as an owned `Arc<Scope>`. Subsequent
|
||||
/// scope mutations do not affect the returned snapshot.
|
||||
pub fn scope_snapshot(&self) -> Arc<Scope> {
|
||||
self.scope.snapshot()
|
||||
}
|
||||
|
||||
/// Apply `extra_allow` to the Pod's runtime scope. Future tool
|
||||
/// permission checks (read/write/glob/grep) reflect the broadened
|
||||
/// scope; in-flight tool calls keep the snapshot they captured at
|
||||
/// invocation time.
|
||||
pub fn add_scope_rules(
|
||||
&self,
|
||||
extra_allow: impl IntoIterator<Item = ScopeRule>,
|
||||
) -> Result<(), ScopeError> {
|
||||
let extra: Vec<ScopeRule> = extra_allow.into_iter().collect();
|
||||
self.scope
|
||||
.update(|cur| cur.with_added_allow_rules(extra.clone()))
|
||||
}
|
||||
|
||||
/// Strip `revoke` rules from the Pod's runtime scope by adding
|
||||
/// matching deny rules. A `Permission::Write` revoke caps effective
|
||||
/// access at `Read` (mirroring the pod-registry `effective_write`
|
||||
/// semantics — Write is the only permission tracked across Pods).
|
||||
/// A `Permission::Read` revoke removes access entirely.
|
||||
pub fn revoke_scope_rules(
|
||||
&self,
|
||||
revoke: impl IntoIterator<Item = ScopeRule>,
|
||||
) -> Result<(), ScopeError> {
|
||||
let revoke: Vec<ScopeRule> = revoke.into_iter().collect();
|
||||
self.scope
|
||||
.update(|cur| cur.with_added_deny_rules(revoke.clone()))
|
||||
}
|
||||
|
||||
/// Direct access to the underlying Worker.
|
||||
pub fn worker(&self) -> &Worker<C, Mutable> {
|
||||
self.worker.as_ref().expect("worker taken during run")
|
||||
|
|
@ -582,10 +623,11 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
} else {
|
||||
None
|
||||
};
|
||||
let scope_snapshot = self.scope.snapshot();
|
||||
let ctx = SystemPromptContext {
|
||||
now: chrono::Utc::now(),
|
||||
cwd: &self.pwd,
|
||||
scope: &self.scope,
|
||||
scope: &scope_snapshot,
|
||||
tool_names,
|
||||
agents_md: agents_md_read.body,
|
||||
resident_knowledge: resident_slice,
|
||||
|
|
@ -667,7 +709,7 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
/// are skipped — the unresolved placeholder stays in the flattened
|
||||
/// user message so the LLM still sees the intent.
|
||||
fn resolve_file_refs(&self, segments: &[Segment]) -> Vec<Item> {
|
||||
let view = crate::fs_view::PodFsView::new(tools::ScopedFs::new(
|
||||
let view = crate::fs_view::PodFsView::new(tools::ScopedFs::with_shared_scope(
|
||||
self.scope.clone(),
|
||||
self.pwd.clone(),
|
||||
));
|
||||
|
|
@ -1078,7 +1120,7 @@ impl<C: LlmClient, St: Store> Pod<C, St> {
|
|||
// with the main Pod (reads go through the same policy) but the
|
||||
// Tracker is fresh — compact-time reads must not pollute the
|
||||
// main session's recency list, which feeds `default_refs` above.
|
||||
let scoped_fs = tools::ScopedFs::new(self.scope.clone(), self.pwd.clone());
|
||||
let scoped_fs = tools::ScopedFs::with_shared_scope(self.scope.clone(), self.pwd.clone());
|
||||
let summary_tracker = tools::Tracker::new();
|
||||
let summary_client: Box<dyn LlmClient> = self.build_compactor_client()?;
|
||||
let summary_system_prompt = self
|
||||
|
|
@ -1801,7 +1843,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
session_id,
|
||||
head_hash: None,
|
||||
pwd: common.pwd,
|
||||
scope: common.scope,
|
||||
scope: SharedScope::new(common.scope),
|
||||
hook_builder: HookRegistryBuilder::new(),
|
||||
interceptor_installed: false,
|
||||
compact_state: None,
|
||||
|
|
@ -1859,7 +1901,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
session_id,
|
||||
head_hash: None,
|
||||
pwd: common.pwd,
|
||||
scope: common.scope,
|
||||
scope: SharedScope::new(common.scope),
|
||||
hook_builder: HookRegistryBuilder::new(),
|
||||
interceptor_installed: false,
|
||||
compact_state: None,
|
||||
|
|
@ -1967,7 +2009,7 @@ impl<St: Store> Pod<Box<dyn LlmClient>, St> {
|
|||
session_id,
|
||||
head_hash: state.head_hash,
|
||||
pwd: common.pwd,
|
||||
scope: common.scope,
|
||||
scope: SharedScope::new(common.scope),
|
||||
hook_builder: HookRegistryBuilder::new(),
|
||||
interceptor_installed: false,
|
||||
compact_state: None,
|
||||
|
|
|
|||
|
|
@ -15,7 +15,7 @@ use async_trait::async_trait;
|
|||
use llm_worker::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput};
|
||||
use manifest::{
|
||||
ModelManifest, Permission, PodManifestConfig, PodMetaConfig, ScopeConfig, ScopeRule,
|
||||
WorkerManifestConfig,
|
||||
SharedScope, WorkerManifestConfig,
|
||||
};
|
||||
use protocol::Method;
|
||||
use protocol::stream::JsonLineWriter;
|
||||
|
|
@ -119,6 +119,14 @@ pub struct SpawnPodTool {
|
|||
/// configuration in the manifest cascade. Per-spawn override is
|
||||
/// out of scope here (see `tickets/spawn-inherit-provider.md`).
|
||||
spawner_model: ModelManifest,
|
||||
/// Spawner's runtime scope. After a successful spawn, the
|
||||
/// `Permission::Write` rules in the delegated scope are revoked
|
||||
/// from the spawner's in-memory view (a `deny(Write, target)` is
|
||||
/// pushed on top, downgrading the spawner's effective access on
|
||||
/// those paths to `Read`). Mirrors the pod-registry's
|
||||
/// `effective_write` semantics: Write is the only permission
|
||||
/// tracked across Pods, so revocation only touches Write.
|
||||
spawner_scope: SharedScope,
|
||||
}
|
||||
|
||||
impl SpawnPodTool {
|
||||
|
|
@ -130,6 +138,7 @@ impl SpawnPodTool {
|
|||
registry: Arc<SpawnedPodRegistry>,
|
||||
parent_socket: Option<PathBuf>,
|
||||
spawner_model: ModelManifest,
|
||||
spawner_scope: SharedScope,
|
||||
) -> Self {
|
||||
Self {
|
||||
spawner_name,
|
||||
|
|
@ -139,6 +148,7 @@ impl SpawnPodTool {
|
|||
registry,
|
||||
parent_socket,
|
||||
spawner_model,
|
||||
spawner_scope,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -217,6 +227,27 @@ impl Tool for SpawnPodTool {
|
|||
|
||||
// Child is live. Post-start errors propagate but do not roll
|
||||
// back the scope allocation — the child already owns it.
|
||||
//
|
||||
// Mirror that ownership transfer in the spawner's in-memory
|
||||
// scope: every `Permission::Write` rule in the delegated scope
|
||||
// is shadowed by a `deny(Write, target)` so subsequent tool
|
||||
// calls (Edit/Write) on the delegated paths fail with
|
||||
// `ReadOnly`. Read access is left intact — the registry only
|
||||
// arbitrates Write, and keeping Read lets the spawner observe
|
||||
// the child's intermediate output through Read/Glob/Grep.
|
||||
let revoke_write: Vec<ScopeRule> = scope_allow
|
||||
.iter()
|
||||
.filter(|r| r.permission == Permission::Write)
|
||||
.cloned()
|
||||
.collect();
|
||||
if !revoke_write.is_empty() {
|
||||
self.spawner_scope
|
||||
.update(|cur| cur.with_added_deny_rules(revoke_write.clone()))
|
||||
.map_err(|e| {
|
||||
ToolError::ExecutionFailed(format!("revoke spawner scope: {e}"))
|
||||
})?;
|
||||
}
|
||||
|
||||
send_run(&predicted_socket, &input.task).await?;
|
||||
|
||||
let record = SpawnedPodRecord {
|
||||
|
|
@ -456,6 +487,7 @@ pub fn spawn_pod_tool(
|
|||
registry: Arc<SpawnedPodRegistry>,
|
||||
parent_socket: Option<PathBuf>,
|
||||
spawner_model: ModelManifest,
|
||||
spawner_scope: SharedScope,
|
||||
) -> ToolDefinition {
|
||||
Arc::new(move || {
|
||||
let schema = schemars::schema_for!(SpawnPodInput);
|
||||
|
|
@ -471,6 +503,7 @@ pub fn spawn_pod_tool(
|
|||
registry.clone(),
|
||||
parent_socket.clone(),
|
||||
spawner_model.clone(),
|
||||
spawner_scope.clone(),
|
||||
));
|
||||
(meta, tool)
|
||||
})
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@ use std::path::{Path, PathBuf};
|
|||
use std::sync::{LazyLock, Mutex};
|
||||
|
||||
use llm_worker::tool::{ToolError, ToolOutput};
|
||||
use manifest::{AuthRef, ModelManifest, Permission, SchemeKind, ScopeRule};
|
||||
use manifest::{AuthRef, ModelManifest, Permission, SchemeKind, Scope, ScopeRule, SharedScope};
|
||||
use pod::runtime::dir::{RuntimeDir, SpawnedPodRecord};
|
||||
use pod::runtime::pod_registry::{self, LockFileGuard};
|
||||
use pod::spawn::registry::SpawnedPodRegistry;
|
||||
|
|
@ -149,6 +149,14 @@ fn dummy_model() -> ModelManifest {
|
|||
}
|
||||
}
|
||||
|
||||
/// Spawner-side `SharedScope` mirroring the `allow_root` granted by
|
||||
/// `setup_spawner`. The tool revokes Write rules from this scope on
|
||||
/// successful spawn — tests can `load()` it to assert the
|
||||
/// revocation took effect.
|
||||
fn shared_scope_for(allow_root: &Path) -> SharedScope {
|
||||
SharedScope::new(Scope::writable(allow_root).unwrap())
|
||||
}
|
||||
|
||||
fn clear_env() {
|
||||
unsafe {
|
||||
std::env::remove_var("INSOMNIA_RUNTIME_DIR");
|
||||
|
|
@ -169,6 +177,7 @@ async fn spawn_pod_delegates_scope_and_sends_run() {
|
|||
let received = accept_one_method(listener);
|
||||
|
||||
let registry = SpawnedPodRegistry::new(spawner_rd.clone());
|
||||
let spawner_scope = shared_scope_for(allow_root.path());
|
||||
let def = spawn_pod_tool(
|
||||
"root".into(),
|
||||
spawner_socket.clone(),
|
||||
|
|
@ -177,6 +186,7 @@ async fn spawn_pod_delegates_scope_and_sends_run() {
|
|||
registry,
|
||||
None,
|
||||
dummy_model(),
|
||||
spawner_scope.clone(),
|
||||
);
|
||||
let (_meta, tool) = def();
|
||||
|
||||
|
|
@ -190,6 +200,13 @@ async fn spawn_pod_delegates_scope_and_sends_run() {
|
|||
})
|
||||
.to_string();
|
||||
|
||||
// Pre-spawn: the spawner can write to the delegated path.
|
||||
assert!(
|
||||
spawner_scope
|
||||
.load()
|
||||
.is_writable(&allow_root.path().join("a.txt"))
|
||||
);
|
||||
|
||||
let output: ToolOutput = tool.execute(&input).await.unwrap();
|
||||
assert!(
|
||||
output.summary.contains("child"),
|
||||
|
|
@ -225,6 +242,15 @@ async fn spawn_pod_delegates_scope_and_sends_run() {
|
|||
assert_eq!(records[0].pod_name, "child");
|
||||
assert_eq!(records[0].callback_address, spawner_socket);
|
||||
|
||||
// Post-spawn: the spawner's runtime scope has been demoted on the
|
||||
// delegated path. Write is gone, Read remains.
|
||||
let post = spawner_scope.load();
|
||||
assert_eq!(
|
||||
post.permission_at(&allow_root.path().join("a.txt")),
|
||||
Some(Permission::Read),
|
||||
"spawner should still be able to read delegated path"
|
||||
);
|
||||
|
||||
clear_env();
|
||||
}
|
||||
|
||||
|
|
@ -239,6 +265,7 @@ async fn spawn_pod_rejects_scope_outside_spawner() {
|
|||
point_pod_command_at_true();
|
||||
|
||||
let registry = SpawnedPodRegistry::new(spawner_rd);
|
||||
let spawner_scope = shared_scope_for(allow_root.path());
|
||||
let def = spawn_pod_tool(
|
||||
"root".into(),
|
||||
spawner_socket,
|
||||
|
|
@ -247,6 +274,7 @@ async fn spawn_pod_rejects_scope_outside_spawner() {
|
|||
registry,
|
||||
None,
|
||||
dummy_model(),
|
||||
spawner_scope.clone(),
|
||||
);
|
||||
let (_meta, tool) = def();
|
||||
|
||||
|
|
@ -277,6 +305,13 @@ async fn spawn_pod_rejects_scope_outside_spawner() {
|
|||
let guard = LockFileGuard::open(&lock_path).unwrap();
|
||||
assert!(guard.data().find("child").is_none());
|
||||
|
||||
// Failed spawn must not have demoted the spawner's scope either.
|
||||
assert!(
|
||||
spawner_scope
|
||||
.load()
|
||||
.is_writable(&allow_root.path().join("a.txt"))
|
||||
);
|
||||
|
||||
clear_env();
|
||||
}
|
||||
|
||||
|
|
@ -301,6 +336,7 @@ async fn spawn_pod_rolls_back_reservation_when_socket_never_appears() {
|
|||
// by running this test alone when iterating.
|
||||
|
||||
let registry = SpawnedPodRegistry::new(spawner_rd);
|
||||
let spawner_scope = shared_scope_for(allow_root.path());
|
||||
let def = spawn_pod_tool(
|
||||
"root".into(),
|
||||
spawner_socket,
|
||||
|
|
@ -309,6 +345,7 @@ async fn spawn_pod_rolls_back_reservation_when_socket_never_appears() {
|
|||
registry,
|
||||
None,
|
||||
dummy_model(),
|
||||
spawner_scope.clone(),
|
||||
);
|
||||
let (_meta, tool) = def();
|
||||
|
||||
|
|
@ -341,5 +378,13 @@ async fn spawn_pod_rolls_back_reservation_when_socket_never_appears() {
|
|||
"allocation was not rolled back after socket wait timed out"
|
||||
);
|
||||
|
||||
// Spawner's runtime scope must also be untouched — revoke is
|
||||
// performed only after exec_child succeeds.
|
||||
assert!(
|
||||
spawner_scope
|
||||
.load()
|
||||
.is_writable(&allow_root.path().join("a.txt"))
|
||||
);
|
||||
|
||||
clear_env();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -13,17 +13,23 @@ use std::io::Write as _;
|
|||
use std::path::{Path, PathBuf};
|
||||
use std::sync::Arc;
|
||||
|
||||
use manifest::Scope;
|
||||
use manifest::{Scope, SharedScope};
|
||||
|
||||
use crate::error::ToolsError;
|
||||
|
||||
#[derive(Debug)]
|
||||
struct ScopedFsInner {
|
||||
scope: Scope,
|
||||
scope: SharedScope,
|
||||
pwd: PathBuf,
|
||||
}
|
||||
|
||||
/// Scope-aware filesystem handle. Clone-cheap (`Arc` inside).
|
||||
///
|
||||
/// The wrapped [`SharedScope`] is shared with every clone of this
|
||||
/// `ScopedFs` and with whoever else holds the same `SharedScope`
|
||||
/// handle (typically the owning Pod). Mutations to that `SharedScope`
|
||||
/// propagate atomically; the next permission check inside any
|
||||
/// `ScopedFs` reads the new view.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct ScopedFs {
|
||||
inner: Arc<ScopedFsInner>,
|
||||
|
|
@ -37,15 +43,34 @@ pub struct WriteOutcome {
|
|||
}
|
||||
|
||||
impl ScopedFs {
|
||||
/// Create a new [`ScopedFs`] wrapping the given [`Scope`] and pwd.
|
||||
/// Create a new [`ScopedFs`] wrapping `scope` and `pwd` in a fresh
|
||||
/// [`SharedScope`]. Use [`ScopedFs::with_shared_scope`] when you
|
||||
/// need the resulting `ScopedFs` to share scope state with another
|
||||
/// holder of the `SharedScope` (typically the Pod).
|
||||
pub fn new(scope: Scope, pwd: PathBuf) -> Self {
|
||||
Self::with_shared_scope(SharedScope::new(scope), pwd)
|
||||
}
|
||||
|
||||
/// Build a [`ScopedFs`] over an existing [`SharedScope`]. The
|
||||
/// resulting handle and any future updates the caller pushes to
|
||||
/// `scope` are observed by every clone of this `ScopedFs`.
|
||||
pub fn with_shared_scope(scope: SharedScope, pwd: PathBuf) -> Self {
|
||||
Self {
|
||||
inner: Arc::new(ScopedFsInner { scope, pwd }),
|
||||
}
|
||||
}
|
||||
|
||||
/// The underlying [`Scope`].
|
||||
pub fn scope(&self) -> &Scope {
|
||||
/// Snapshot the current scope. Cheap; the returned `Arc<Scope>` is
|
||||
/// a coherent point-in-time view that subsequent mutations do not
|
||||
/// affect.
|
||||
pub fn scope(&self) -> Arc<Scope> {
|
||||
self.inner.scope.snapshot()
|
||||
}
|
||||
|
||||
/// Shared scope handle backing this `ScopedFs`. Cloning it lets a
|
||||
/// caller (usually the Pod) hold the same view and push updates
|
||||
/// that are immediately reflected in subsequent permission checks.
|
||||
pub fn shared_scope(&self) -> &SharedScope {
|
||||
&self.inner.scope
|
||||
}
|
||||
|
||||
|
|
@ -67,7 +92,7 @@ impl ScopedFs {
|
|||
if !path.is_absolute() {
|
||||
return Err(ToolsError::RelativePath(path.to_path_buf()));
|
||||
}
|
||||
if !self.inner.scope.is_readable(path) {
|
||||
if !self.inner.scope.load().is_readable(path) {
|
||||
return Err(ToolsError::OutOfScope(path.to_path_buf()));
|
||||
}
|
||||
let meta = std::fs::metadata(path).map_err(|e| match e.kind() {
|
||||
|
|
@ -100,13 +125,15 @@ impl ScopedFs {
|
|||
if !path.is_absolute() {
|
||||
return Err(ToolsError::RelativePath(path.to_path_buf()));
|
||||
}
|
||||
if !self.inner.scope.is_writable(path) {
|
||||
return Err(if self.inner.scope.is_readable(path) {
|
||||
let scope = self.inner.scope.load();
|
||||
if !scope.is_writable(path) {
|
||||
return Err(if scope.is_readable(path) {
|
||||
ToolsError::ReadOnly(path.to_path_buf())
|
||||
} else {
|
||||
ToolsError::OutOfScope(path.to_path_buf())
|
||||
});
|
||||
}
|
||||
drop(scope);
|
||||
|
||||
// Reject existing directory targets.
|
||||
match std::fs::metadata(path) {
|
||||
|
|
@ -299,4 +326,118 @@ mod tests {
|
|||
let err = fs.write(dir.path(), b"x").unwrap_err();
|
||||
assert!(matches!(err, ToolsError::IsDirectory(_)));
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
// Dynamic scope: SharedScope mutations propagate into ScopedFs decisions
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
#[test]
|
||||
fn add_allow_rule_through_shared_scope_grows_readable_set() {
|
||||
use manifest::SharedScope;
|
||||
|
||||
let dir = TempDir::new().unwrap();
|
||||
let extra = TempDir::new().unwrap();
|
||||
let extra_file = extra.path().join("x.txt");
|
||||
fs::write(&extra_file, b"hi").unwrap();
|
||||
|
||||
let shared = SharedScope::new(Scope::writable(dir.path()).unwrap());
|
||||
let fs = ScopedFs::with_shared_scope(shared.clone(), dir.path().to_path_buf());
|
||||
|
||||
// Before: extra is out of scope.
|
||||
let err = fs.read_bytes(&extra_file).unwrap_err();
|
||||
assert!(matches!(err, ToolsError::OutOfScope(_)));
|
||||
|
||||
// Push an allow(Read) rule.
|
||||
shared
|
||||
.update(|cur| {
|
||||
cur.with_added_allow_rules([ScopeRule {
|
||||
target: extra.path().to_path_buf(),
|
||||
permission: Permission::Read,
|
||||
recursive: true,
|
||||
}])
|
||||
})
|
||||
.unwrap();
|
||||
|
||||
// After: read goes through.
|
||||
assert_eq!(fs.read_bytes(&extra_file).unwrap(), b"hi");
|
||||
// But write still fails — allow only granted Read.
|
||||
let err = fs.write(&extra.path().join("y.txt"), b"x").unwrap_err();
|
||||
assert!(
|
||||
matches!(err, ToolsError::ReadOnly(_)),
|
||||
"expected ReadOnly, got {err:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn revoke_write_through_shared_scope_blocks_subsequent_writes() {
|
||||
use manifest::SharedScope;
|
||||
|
||||
let dir = TempDir::new().unwrap();
|
||||
let sub = dir.path().join("sub");
|
||||
fs::create_dir(&sub).unwrap();
|
||||
let target = sub.join("a.txt");
|
||||
|
||||
let shared = SharedScope::new(Scope::writable(dir.path()).unwrap());
|
||||
let fs = ScopedFs::with_shared_scope(shared.clone(), dir.path().to_path_buf());
|
||||
|
||||
// Write succeeds initially.
|
||||
fs.write(&target, b"first").unwrap();
|
||||
|
||||
// Revoke Write on `sub` (push a deny(Write) rule).
|
||||
shared
|
||||
.update(|cur| {
|
||||
cur.with_added_deny_rules([ScopeRule {
|
||||
target: sub.clone(),
|
||||
permission: Permission::Write,
|
||||
recursive: true,
|
||||
}])
|
||||
})
|
||||
.unwrap();
|
||||
|
||||
// Subsequent write fails with ReadOnly — Read is preserved.
|
||||
let err = fs.write(&target, b"second").unwrap_err();
|
||||
assert!(
|
||||
matches!(err, ToolsError::ReadOnly(_)),
|
||||
"expected ReadOnly after revoke, got {err:?}"
|
||||
);
|
||||
// Read still works.
|
||||
assert_eq!(fs.read_bytes(&target).unwrap(), b"first");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shared_scope_changes_propagate_across_clones() {
|
||||
use manifest::SharedScope;
|
||||
|
||||
let dir = TempDir::new().unwrap();
|
||||
let target = dir.path().join("a.txt");
|
||||
|
||||
let shared = SharedScope::new(Scope::writable(dir.path()).unwrap());
|
||||
let fs1 = ScopedFs::with_shared_scope(shared.clone(), dir.path().to_path_buf());
|
||||
let fs2 = fs1.clone();
|
||||
|
||||
// fs1 writes; both clones see the file.
|
||||
fs1.write(&target, b"hi").unwrap();
|
||||
assert_eq!(fs2.read_bytes(&target).unwrap(), b"hi");
|
||||
|
||||
// Revoke write through the original handle.
|
||||
shared
|
||||
.update(|cur| {
|
||||
cur.with_added_deny_rules([ScopeRule {
|
||||
target: dir.path().to_path_buf(),
|
||||
permission: Permission::Write,
|
||||
recursive: true,
|
||||
}])
|
||||
})
|
||||
.unwrap();
|
||||
|
||||
// Both clones reject writes now — they share the same SharedScope.
|
||||
assert!(matches!(
|
||||
fs1.write(&target, b"x").unwrap_err(),
|
||||
ToolsError::ReadOnly(_)
|
||||
));
|
||||
assert!(matches!(
|
||||
fs2.write(&target, b"x").unwrap_err(),
|
||||
ToolsError::ReadOnly(_)
|
||||
));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,38 +0,0 @@
|
|||
# Bash ツール
|
||||
|
||||
## 背景
|
||||
|
||||
builtin-tools チケットで Read/Write/Edit/Glob/Grep の5ツールは実装済み。
|
||||
Bash ツールは子プロセスが直接 fs を触るため ScopedFs では保護できず、
|
||||
Permission 層（deny/allow ルール）との統合が前提。
|
||||
|
||||
## 実装内容
|
||||
|
||||
- コマンド実行（`tokio::process::Command`）
|
||||
- タイムアウト（`timeout` パラメータ、デフォルト 120秒、最大 600秒）
|
||||
- stateless: 各呼び出しは workspace root から fresh start。pwd は session を跨いで保持しない（`cd <dir> && cmd` のように chain させる運用。Claude Code と同方針）
|
||||
- stdout/stderr の結合出力
|
||||
- ToolOutput の summary（コマンド + exit code）+ content（出力テキスト）
|
||||
- 出力ハンドリング: 短い場合（≤ 80行 & ≤ 12 KiB）はインラインで返す。それを超えたら full output を `<runtime_dir>/<pod_name>/bash-output/` 配下のファイルに退避し、tail 80行 + ファイルパスを返却。Worker の `ToolOutputLimits` (default 16 KiB) が末尾を切り捨てる挙動を回避するため
|
||||
- 退避ファイルは scope に `allow(Read)` で追加するので `Read` ツールで読める。Pod 終了で `RuntimeDir::Drop` がまとめて掃除、session 終了でも `BashTool::Drop` が個別削除
|
||||
|
||||
## Scope との関係
|
||||
|
||||
Bash の子プロセスは ScopedFs を経由しない。Scope による保護は不可能。
|
||||
|
||||
代わりに:
|
||||
- `PreToolCall` Hook + Permission ルール（`[permission]` マニフェストセクション）で制御
|
||||
- Permission 未実装の間は制約なしで動作
|
||||
|
||||
## 依存チケット
|
||||
|
||||
- [permission-extension-point.md](permission-extension-point.md) — deny/allow ルールによる Bash コマンド制御
|
||||
|
||||
## Review
|
||||
- 状態: Approve with follow-up（Round 2）
|
||||
- レビュー詳細: [./bash-tool.review.md](./bash-tool.review.md)
|
||||
- 日付: 2026-05-01（Round 2: 2026-05-01）
|
||||
- Round 2 残課題:
|
||||
- ~~チケット本文「作業ディレクトリの永続」の記述を stateless 仕様に更新~~（解消済み）
|
||||
- `crates/tools/src/lib.rs:51` の broken intra-doc link 修正（別コミットで対応）
|
||||
- TUI `render_default` 回帰テストは別チケットへ切り出し
|
||||
|
|
@ -1,127 +0,0 @@
|
|||
# Review: Bash ツール
|
||||
|
||||
## Round 1 (初回レビュー)
|
||||
|
||||
### 前提・要件の確認
|
||||
|
||||
- **コマンド実行 (`tokio::process::Command`)**: 満たされている。`crates/tools/src/bash.rs:94-103` で `bash -c <wrapped>` を起動。`stdin(null)` で stdin ブロックを防止、`kill_on_drop(true)` でタイムアウト時のリーク防止。
|
||||
- **timeout (default 120s / max 600s)**: 満たされている。`bash.rs:38-39, 64-67` の `clamp(1, 600)`、`bash.rs:130-144` の `tokio::time::timeout`。`timeout_kills_long_command` で動作確認済み。
|
||||
- **作業ディレクトリの永続**: 満たされている。`cd` のパースに頼らず wrapper script + tempfile で post-command の `pwd` を取得（`bash.rs:74-90`）。`cd_persists_across_calls` テストで `subdir` 移動後の `pwd` が反映されることを確認。`canonicalize` 同士で比較しており macOS の `/private/tmp` ずれにも耐性あり。
|
||||
- **stdout/stderr 結合**: 満たされている。wrapper 内 `exec 2>&1` で実装、`merges_stdout_and_stderr` テストで両方含まれることを確認。子プロセス側の `stderr(Stdio::null())` も整合。
|
||||
- **`ToolOutput` summary（コマンド + exit code）+ content（出力）**: 満たされている。`bash.rs:164-175` で exit 0 / 非 0 / シグナルを区別。content が空のときは `None` を返しており、`SUMMARY_THRESHOLD` を意識した良い実装。
|
||||
|
||||
### アーキテクチャ・スコープ
|
||||
|
||||
- **層分離**: `tools` クレート内に閉じており、`llm-worker` を低レベル基盤に保つ方針と整合（`bash.rs:20` で `Tool` trait のみ依存）。`builtin_tools()` のファクトリ列に追加するだけで、層を跨ぐ侵入はない。
|
||||
- **クレート命名/構造**: `bash.rs` を独立モジュールに切り出し、`lib.rs` で `pub use bash::bash_tool` のみ公開。`read.rs/write.rs/...` と一貫。
|
||||
- **依存追加**: `Cargo.toml` の tokio features に `process`/`time`/`io-util`/`sync` を追加（`Cargo.toml:22`）。`tempfile` は既存。`cargo add` 経由前提のフィールド追加で違和感なし。
|
||||
- **Permission 層との関係**: ticket の前提通り、ScopedFs では保護せず Permission 層に委譲。`lib.rs:18-19` のドキュメントコメントで明示しており、設計意図は読み手に伝わる。
|
||||
- **設計判断 1（wrapper による pwd 取得）**: `cd` パースの脆さ（サブシェル、変数展開、関数定義内 `cd` 等）を回避できるので妥当。`exec` で bash 自体が置換されると wrapper が走らないが、`bash.rs:149-155` が「ファイル読めなければ pwd 据え置き」とフェイルソフトしておりロバスト。
|
||||
- **設計判断 2（wrapper の `wait`）**: `(sleep 0.05; echo bg) &` のようなジョブで stdout が EOF せずハングする問題に対する実装上必須の対処。`background_job_does_not_hang` で回帰防止済み。
|
||||
- **設計判断 3（`tokio::sync::Mutex` で逐次化）**: pwd の共有可変状態と「順序のある shell セッション」の意味論を考えると正解。長時間コマンドの間 lock を握り続けるのは仕様上自然（同一セッションの bash は元々直列）。
|
||||
- **設計判断 4（256KB cap）**: worker 側 `ToolOutputLimits` の手前で OOM を抑える二重防壁。truncated marker の追記後に `String::from_utf8_lossy` で UTF-8 化しており、マルチバイト切断もロスレスではないが panic はしない。妥当。
|
||||
- **設計判断 5（summary/content）**: 既存ツールと API 形状が一致。`SUMMARY_THRESHOLD` の境界も意識されている。
|
||||
- **設計判断 6（description のプロンプト誘導）**: Read/Write/Edit/Glob/Grep を優先させる文言は、Claude Code リファレンスとも整合し、ローカルモデルでも効きやすい簡潔さ。
|
||||
|
||||
### 指摘事項
|
||||
|
||||
#### Non-blocking / Follow-up
|
||||
|
||||
- **TUI 側の `render_default` 修正の同梱について** (`crates/tui/src/tool.rs:590-619`)
|
||||
- 内容としては正しいバグ修正。Bash のような汎用ツールが Detail モードでも summary しか出ない状態を解消している。
|
||||
- ただし、厳密には Bash チケットの範囲外（既存の任意の "default 経路の" ツールに同じ問題があったはず）。同梱の妥当性: Bash 投入によりバグが顕在化したこと、5 行程度の置き換えで完結すること、Bash 単体だと UX として未完であることを踏まえれば現実的な判断と言える。次回同種の状況では、TUI 表示仕様の修正として別チケットを切るほうがレビュー単位がきれいになる、というレベル。
|
||||
- フォローアップ提案: `crates/tui/` 配下に `output` を含むレンダリングが Detail/Normal で正しく出ることを確認するスナップショット/ユニットテストを 1 本追加すると、将来の `summary` フォールバック方向への意図しない退行を防げる（現状はロジックレビューのみで担保）。
|
||||
|
||||
- **`docs/ref/claude-code-deferred-tools.md` への追記**: Bash 実装と直接関係しない文献参照の追加（Anthropic vs OpenAI 比較への言及）。1 段落で軽微とはいえ、チケットスコープからは外れている。次回はドキュメント更新も別コミット/別チケット推奨。
|
||||
|
||||
- **pwd 更新の堅牢性についての観察 (`bash.rs:149-155`)**: ユーザーコマンドが `exec some-program` で bash を置換した場合や、wrapper の `pwd > tempfile` がディスクフル等で失敗した場合に pwd が据え置かれる挙動になっている。仕様上は妥当だが、ユーザー視点では「`cd foo && exec bar` 後に `cd` が消えた」ように見える可能性がある。コメントで現挙動の合理性は説明されているので blocking ではないが、将来 Permission 層導入時にエッジケースとして再考の余地あり。
|
||||
|
||||
#### Nits
|
||||
|
||||
- `BashParams` の `timeout` フィールドが `Option<u64>` で `#[serde(default)]` だが、`Option` は serde が自動的に欠落を `None` にするため `#[serde(default)]` は冗長（害はない）。
|
||||
- `bash.rs:111-112` の `let mut child = child; let mut stdout = stdout;` は `async move` ブロックで mutable に再束縛しているだけ。慣用的だが `let mut` を引数側で書いてもよい。スタイル差。
|
||||
|
||||
### 判断
|
||||
|
||||
**Approve with follow-up** — チケット要件は完全に満たされており、設計判断もすべて合理的に説明されている。テストカバレッジ (8 unit + 1 integration) も妥当。同梱されている TUI 修正は実害のあるバグ修正で内容は正しいが、本来は別チケット相当のスコープ越えがあり、回帰テストの追加は次回までのフォローアップとして残しておくとよい。
|
||||
|
||||
---
|
||||
|
||||
## Round 2 (再レビュー: 2026-05-01)
|
||||
|
||||
### 主な変更点
|
||||
|
||||
1. 256KB byte-cap → **80 行 / 12 KiB を超えたら `<runtime_dir>/<pod_name>/bash-output/bash-XXX.log` に退避、tail 80 行 + パスを返す**
|
||||
2. spill ファイルを Read で読めるようにするため、`Scope::deny_rules()` を `allow_rules()` の対称として追加。controller では memory 流儀（`ScopeConfig` を組み立てて `Scope::from_config` で rebuild）に統一
|
||||
3. **pwd 永続を撤廃**（stateless 化）。各呼び出しは workspace root からスタート。tokio の `sync` feature 削除。`cd_persists_across_calls` → `cd_does_not_persist_across_calls` にテスト反転
|
||||
4. TUI `render_default` の修正は前回と同じまま同梱（回帰テストはまだ）
|
||||
|
||||
### 前提・要件の再確認（仕様変更分）
|
||||
|
||||
- **要件「作業ディレクトリの永続」が撤回されたこと**: チケット本文 (`tickets/bash-tool.md:13`) には依然「作業ディレクトリの永続（ツール内部で `pwd` 状態を保持、`cd` で変更可能）」と書かれている。実装は stateless に切り替わっており、要件文と実装が乖離している。Claude Code 互換のために stateless が正しい判断という認識には同意するが、**チケット本文の更新がない点はチケットライフサイクルの観点で要修正**（`b. 詳細化や前提の変化` 段に該当）。
|
||||
- **stdout/stderr 結合と timeout/exit handling**: 不変。`merges_stdout_and_stderr` / `nonzero_exit_is_reported` / `timeout_kills_long_command` で担保。
|
||||
- **長い出力の扱い**: 新仕様（80 行 / 12 KiB 閾値、tail 返却 + spill パス通知）は description (`bash.rs:42-45`) でモデルに伝達済み。`long_output_spills_and_returns_tail` / `wide_short_output_still_spills_when_byte_budget_exceeded` で行ベース・バイトベース双方の閾値を確認。
|
||||
- **spill ファイルの可視性**: `bash_spilled_file_is_readable_via_read_tool` (integration:353) で「spill 経由で書かれたファイルが Read ツールで読める」E2E が確認できている。Worker の 16 KiB cap で tail が落ちる問題への対処として筋の通った再設計。
|
||||
|
||||
### 設計判断の検証
|
||||
|
||||
#### (1) 二重防衛 cleanup（BashTool::Drop + RuntimeDir::Drop）
|
||||
|
||||
- **適切。** 役割分離が明快:
|
||||
- `BashTool::Drop` (`bash.rs:92-100`) — セッション終了時、その session が積んだ spill 群を削除。controller 上で worker/tools が落ちる経路に乗る。
|
||||
- `RuntimeDir::Drop` (`runtime/dir.rs:103-107`) — pod 終了時、`bash-output/` ディレクトリごと一掃。`SocketServer`、status/history 等と同列の sweeper として機能。
|
||||
- 二段にする必要性も理屈が通る: BashTool だけだと「クラッシュや中断で Drop が走らなかった残骸」が残り続ける可能性があるが、RuntimeDir 全消しが最後の保険になる。逆に RuntimeDir だけだと「同一 pod 内で session を多数立ち上げ続けた場合に bash-output が肥大化する」リスクがあるが、BashTool::Drop が切り詰める。
|
||||
- 設計コメント (`bash.rs:86-88`) で「session 終了時の遅延 cleanup」だと明示しているのも良い。
|
||||
|
||||
#### (2) `Scope::deny_rules()` 追加と memory 流儀への統一
|
||||
|
||||
- **対称性として自然。** `allow_rules()` (`scope.rs:148-157`) と `deny_rules()` (`scope.rs:165-174`) は完全な双子で、ResolvedRule から ScopeRule への射影がペアで揃った形。
|
||||
- 当初考えていた `Scope::with_extra_read(self, PathBuf) -> Self` を撤回したのは正解。`with_extra_*` 系の単機能 API はパス追加のたびに増える危険があり、`ScopeConfig` 経由のラウンドトリップに比べて表現力で劣る。汎用 accessor を 1 本足すだけのほうが余計な API を呼ばない（YAGNI に沿う）。
|
||||
- controller での組み立て方 (`controller.rs:245-255`) は memory の `build_scope_with_memory` (`pod.rs:2030-2037`) と同型で、コメント (`controller.rs:236-237`) で「memory が取っているのと同じアプローチ」と明示しているので意図が伝わりやすい。
|
||||
- `Scope::summary()` (`scope.rs:198-230`) が deny を表示しないのと対称に、`deny_rules()` は明示的にプログラム的取得を許す形になっており、API 設計の意図が一貫している。
|
||||
|
||||
#### (3) stateless 化 + `BashTool.cwd: PathBuf` 不変フィールド
|
||||
|
||||
- **stateless 化自体は正しい判断。** Claude Code 仕様への寄せが理にかなっており、(a) tokio の `sync` feature が落ちて concurrent 実行が可能になる、(b) wrapper から pwd marker tempfile + `pwd > file` 行が消えてシンプル化、(c) サブシェル / 関数定義内 `cd` のような pwd 抽出のエッジケースが消える、と複数の便益がある。
|
||||
- **`cwd: PathBuf` 不変フィールドの妥当性**: 現状の実装で十分。ご質問の「`fs` ごと持って `fs.pwd()` を毎回参照すべきか」については以下の理由で「**現状のスナップショットで OK**」と判断する:
|
||||
- `ScopedFs::pwd()` は pod-lifetime の不変値で、`ScopedFs::new(scope, pwd)` で構築後に変わらない。`fs` をフィールドとして持っても毎回同じ値が返る。
|
||||
- `BashTool` は子プロセスで fs を直接触らない（ScopedFs バイパス）ので、`fs` を参照する用事はない。pwd だけが必要。
|
||||
- 不要なフィールドを抱えるとライフタイムや clone コストが増える。`PathBuf` は単純で読みやすい。
|
||||
- 将来 `Bash` を ScopedFs 経由に切り替える計画があれば `fs` を持つ必要が出てくるが、ticket の前提（Permission 層に委譲）を覆さない限り発生しない。
|
||||
- 一点だけ気になるとすれば、`bash.rs:78-81` のコメントで「`ScopedFs::pwd()` の snapshot」と説明している割に factory (`bash.rs:330-344`) では `fs.pwd().to_path_buf()` を呼ぶだけで、`fs` 自体は捨てているので、もし将来 Permission 層連携で `fs` が要るなら API 形状が変わる。今は気にしなくていい。
|
||||
|
||||
#### (4) TUI 修正の扱い
|
||||
|
||||
- **前回と完全に同一。回帰テスト追加もなし。** 前回 follow-up の 1 つ目「`crates/tui/` 配下に snapshot/unit test を 1 本」は未対応。
|
||||
- 判断: **後段送り（別チケット）が妥当**。今回の追加変更が TUI には触れていない以上、再度 blocking 化するのは過剰。Bash チケットを閉じて、TUI の `render_default` 仕様確認を別チケットに切り出すほうが履歴が綺麗。
|
||||
- ただし「別チケット化を約束する」のは現実的に flaky なので、`TODO.md` への 1 行追加（例: `- TUI tool render_default の output 表示に対する回帰テスト`）または専用チケット作成を Round 2 完了条件に組み込むことを推奨。
|
||||
|
||||
### 新規指摘事項
|
||||
|
||||
#### Blocking
|
||||
|
||||
なし。
|
||||
|
||||
#### Non-blocking / Follow-up
|
||||
|
||||
- **チケット本文と実装の乖離 (`tickets/bash-tool.md:13`)**: 「作業ディレクトリの永続」と書かれているが実装は stateless。Round 2 で意識的に撤回された変更なので、ticket 本文を更新してから完了に進むのが筋。代替の表記例:
|
||||
- `作業ディレクトリ: 各呼び出しは workspace root から開始（stateless、Claude Code 互換）。複数ディレクトリで作業するときは "cd <dir> && cmd" でチェイン`
|
||||
- **`crates/tools/src/lib.rs:51` の broken intra-doc link**: `[\`manifest::Scope::with_extra_read\`]` を残したまま。`with_extra_read` は撤回されて存在しない。`cargo doc -p tools --no-deps` で warning が出る:
|
||||
```
|
||||
warning: unresolved link to `manifest::Scope::with_extra_read`
|
||||
--> crates/tools/src/lib.rs:51:12
|
||||
```
|
||||
実害は doc 生成警告のみだが、撤回した API のドキュメント参照が残るのは将来の混乱の元。`(see [\`manifest::Scope::deny_rules\`] / [\`Scope::from_config\`])` 等に書き直すか、リンクを外して文章だけ残すのが妥当。
|
||||
- **TUI `render_default` 回帰テスト未追加**: Round 1 から持ち越し。前述の通り別チケット化推奨。
|
||||
- **チケット範囲外の `docs/ref/claude-code-deferred-tools.md` 追記**: Round 1 と同じ。Round 2 では新たな逸脱はなし。
|
||||
- **`exec` 系の堅牢性ノート (Round 1 持ち越し)**: stateless 化により pwd 更新の問題が消えたので、Round 1 で挙げた「`cd foo && exec bar` で pwd が消える」観察事項は **解消**。次回 Permission 層導入時のエッジケースとしてのみ意識すれば足りる。
|
||||
|
||||
#### Nits
|
||||
|
||||
- **`BashParams.timeout` の `#[serde(default)]`**: Round 1 と同じく冗長（害はない）。
|
||||
- **`bash.rs:151-163` の child エラー早期 return パス**: `output_path` の cleanup を 3 箇所で重複 (`spawn` 失敗、`wait` 失敗、timeout でファイルサイズ 0)。1 つの helper に括れるが、エラーハンドラの分岐が異なる（`?` で抜けるかそうでないか）ので無理に統一する必要はない。Mio スタイルが統一されていれば良い、というレベル。
|
||||
- **`shell_single_quote`** (`bash.rs:319-322`): 現実的にはここに来る出力パスは tempfile が生成した英数字パスで、`'` を含むことは無いに等しい。とはいえ防御として正しい実装で、コストもゼロ。
|
||||
|
||||
### 判断
|
||||
|
||||
**Approve with follow-up** — Round 2 の主要変更（spill 退避、`deny_rules()`、stateless 化、cleanup 二重化）はいずれも設計として筋が良く、テストカバレッジ（unit 10 / integration 14 / edge_cases 9 全 pass、`cargo check --workspace` クリーン、TUI 55/55 pass）で動作も担保されている。残作業は (a) チケット本文の文言を stateless に合わせて更新、(b) `lib.rs:51` の broken doc link を修正、(c) TUI の回帰テストは別チケットへ切り出し — の 3 点。いずれも軽微で、(a)(b) を Round 2 完了の条件として処理すれば本チケット自体はクローズ可能。
|
||||
|
|
@ -1,45 +0,0 @@
|
|||
# 動的 Scope 変更
|
||||
|
||||
## 背景
|
||||
|
||||
現状の Pod の `Scope` は `Pod::from_manifest` 時に1回構築され、以後 immutable。pod-registry (`crates/pod-registry`) は登録・削除・衝突チェックを任意のタイミングで行えるためレジストリ側の制約はないが、Pod 内部の `Scope` と `ScopedFs` が起動時に固定されているため、実行中に scope を追加・縮小することができない。
|
||||
|
||||
オーケストレーションでは SpawnPod による scope 分譲で effective scope が縮小するが、これは pod-registry 上の記録にとどまり、Pod 側の `ScopedFs` は元の scope のまま動作している（ツール実行時に pod-registry と照合していない）。
|
||||
|
||||
また将来、外部から Pod に scope を動的に付与するケース（人間が「このディレクトリも触っていいよ」と追加する、別 Pod が scope を委譲してくる等）にも対応したい。
|
||||
|
||||
## ゴール
|
||||
|
||||
Pod の実行中に scope を追加・縮小でき、変更が即座にツール実行の permission チェックに反映される。
|
||||
|
||||
## 必要な変更
|
||||
|
||||
### Pod 側
|
||||
|
||||
- `Scope` を `Arc<RwLock<Scope>>`（または同等の共有可変参照）にする
|
||||
- `ScopedFs` が `Scope` の共有参照を持ち、ツール実行時に最新の scope を参照する
|
||||
- scope 変更メソッド: `pod.update_scope(new_scope_config)` → pod-registry 更新 + `Scope` 再構築 + `ScopedFs` に反映
|
||||
|
||||
### pod-registry との連携
|
||||
|
||||
- scope 追加時: `flock → 衝突チェック → 追加分を登録 → unlock → Pod 内 Scope 再構築`
|
||||
- scope 縮小時（分譲）: `flock → 分譲を記録 → unlock → Pod 内 Scope 再構築`
|
||||
- 現在の SpawnPod による分譲を、この汎用パスに統合する
|
||||
|
||||
### protocol 拡張（任意）
|
||||
|
||||
- `Method::GrantScope { scope }` — 外部から Pod に scope を動的付与
|
||||
- `Method::RevokeScope { scope }` — 外部から Pod の scope を縮小
|
||||
- 当面は Pod 内部（SpawnPod ツール）からの変更だけで十分なら不要
|
||||
|
||||
## 完了条件
|
||||
|
||||
- Pod の実行中に scope を追加でき、追加後のツール実行が新しい scope を反映する
|
||||
- Pod の実行中に scope を縮小でき、縮小後のツール実行が制限を反映する
|
||||
- scope 変更が pod-registry と Pod 内 Scope の両方に整合的に反映される
|
||||
- 単体テストで動的追加・縮小後の permission チェックが検証される
|
||||
|
||||
## 範囲外
|
||||
|
||||
- protocol 経由の外部からの scope 付与 / 剥奪（必要になったら追加）
|
||||
- scope 変更の履歴追跡・監査ログ
|
||||
|
|
@ -1,108 +0,0 @@
|
|||
# メモリ機構: Phase 2 consolidation + 整理
|
||||
|
||||
## 背景
|
||||
|
||||
`docs/plan/memory.md` §自動化メカニズム / §整理（GC 相当）の扱い の実装。staging の活動ログ + 既存 `memory/*` + Knowledge 化候補レポート + 整理材料を入力に、consolidation Worker が **統合 phase（活動ログを memory/knowledge へ落とす）** と **整理 phase（既存 record の drop / merge / split / trim / rewrite）** を 1 セッション内で順に実行する。Phase 1 を終えた pod が spawn し、並走防止は staging 配下の進行状況ファイルで担保する。
|
||||
|
||||
整理 phase は Phase 2 とは別 trigger を持たない。同じ Worker 同じツール surface で済むため、別 Agent / 別 spawn 経路は設けない。
|
||||
|
||||
Knowledge 新規作成は「候補レポート掲載の source から派生する場合」に限る。使用頻度メトリクス（候補レポートと保護閾値の集計元）が未完のうちは、レポートと整理材料は空入力として動作し、統合 phase は decisions / requests / summary / 既存 Knowledge update のみ、整理 phase は Linter Warn と `replaced` chain と sources 累積を見るに留まる（保護閾値による drop 抑制は metrics 完成後に有効化）。
|
||||
|
||||
## 要件
|
||||
|
||||
### Trigger
|
||||
|
||||
- staging の累積ファイル数 or bytes 閾値（設定で tune）
|
||||
- compact 同期発火は持たない（raw 保全は Phase 1 が compact 前に走ることで担保される）
|
||||
|
||||
### 実行主体と入力
|
||||
|
||||
- Phase 1 を終えた pod が consolidation Worker を spawn
|
||||
- 起動時スナップショットで consumed ID list を確定
|
||||
- 入力:
|
||||
- consumed ID 分の staging エントリ（活動ログ + `source`）
|
||||
- 既存 `memory/*`（summary / decisions / requests）全文
|
||||
- Knowledge 化候補レポート（メトリクスチケットの成果物。未完のうちは空）
|
||||
- 整理材料（使用頻度メトリクス、Linter Warn、`replaced` chain、sources 過多情報。メトリクス未完のうちは Linter Warn / `replaced` / sources のみ）
|
||||
- 既存 `knowledge/*` は prompt に埋めず、Knowledge 検索ツール経由で agent が引く
|
||||
|
||||
### 渡すツール
|
||||
|
||||
- 汎用 CRUD（file read / write / edit）
|
||||
- memory 検索ツール
|
||||
- Knowledge 検索ツール
|
||||
- post-write Linter Hook（違反時 turn 戻し、N 回失敗 abort）
|
||||
|
||||
### 常駐注入の無効化
|
||||
|
||||
- consolidation Worker の Pod 起動直後に `Pod::set_resident_knowledge_injection(false)` を呼ぶ
|
||||
- `model_invokation: ON` の Knowledge description を system prompt に載せず、Knowledge 検索ツール経由で agent に引かせる方針（本セクション「入力」と整合）
|
||||
- 仕組み自体は `tickets/memory-resident-injection.md` で導入済み。lever は用意されているが Phase 2 spawn 経路がまだないので、本チケットの実装範囲で必ず呼ぶこと
|
||||
|
||||
### 処理内容
|
||||
|
||||
#### 統合 phase
|
||||
|
||||
- 新規 decisions / requests を 1 件 1 ファイルで追加、`sources` は staging の `source` をコピー（LLM 推論ではない）
|
||||
- 活動ログから派生する Knowledge を新規作成 or 既存 patch。**新規作成は候補レポート掲載の source 由来に限る**
|
||||
- summary を必要に応じて rewrite（1-5k tokens 目安）
|
||||
- Decision の置き換えは `status: replaced` + `replaced_by: <slug>`、直接削除しない
|
||||
- 書き込み先: `memory/*`, `knowledge/*`。`memory/workflow/` は Linter で弾かれる
|
||||
|
||||
#### 整理 phase（統合 phase 完了後、余力で実行）
|
||||
|
||||
- 既存 record 群を `outdated | superseded | unused | noisy` の評価カテゴリで分類
|
||||
- 操作粒度はファイル単位（drop / merge / split）とファイル内部分（節・箇条削除、`sources` 古いエントリ trim）
|
||||
- **保護閾値**: 明示 invoke `frequency >= 1.0 invokes/Mtoken` の record は drop / 大幅圧縮の対象外（初期値 1.0、workspace 設定でカスタマイズ可、metrics 未完のうちは閾値判定スキップで保守的に振る舞う）
|
||||
- 単一 record が複数カテゴリに該当してもよい
|
||||
- 直接削除してよいが、誤判定しやすいものは merge / trim を優先（prompt 側で誘導）
|
||||
- Linter Warn で検出した類似 slug 乱立 / sources 過多 / `replaced` 滞留はここで収斂
|
||||
|
||||
### 並走防止
|
||||
|
||||
- staging 配下に 1 ファイル（Pod 識別子 + consumed ID list）
|
||||
- 存在し、示された Pod が動作している間、そのプロセスが排他占有
|
||||
- 実行中に Phase 1 が追加した staging は触らず、次回 Phase 2（Coalesce）に委ねる
|
||||
- 完了時は consumed ID list の staging のみ cleanup、追加分は残す
|
||||
- Phase 2 完了時に staging 新着があれば次を発火（Coalesce）
|
||||
- 占有の実現方法（pid 存在確認 / flock / 他）は実装判断
|
||||
|
||||
### モデル
|
||||
|
||||
- 設定 key `memory.consolidation_model`（reasoning 系）
|
||||
|
||||
### prompt
|
||||
|
||||
- `docs/plan/memory-prompts.md` §共通原則 / §Phase 2: 統合 + 整理 prompt / §Phase 2: Knowledge 書き込み prompt に従う
|
||||
|
||||
## 範囲外
|
||||
|
||||
- 使用頻度メトリクスと Knowledge 化候補レポート / 保護閾値の集計（別チケット。未完の間は空入力で動作）
|
||||
- Workflow 関連の offer（別チケット、Notification 経路が先）
|
||||
- 意味破壊検出の監査 LLM 層（将来検討）
|
||||
|
||||
## 完了条件
|
||||
|
||||
- Phase 1 が staging に残した活動ログを統合 phase が `memory/*` / `knowledge/*` に統合する
|
||||
- 統合 phase 完了後、整理 phase が同じ agent セッション内で続けて走り、既存 record を整理する
|
||||
- Linter 違反時は turn が戻り、sub-Worker が自己修正する
|
||||
- 並走防止ファイルが想定通り機能し、複数 Phase 2 の重複起動が防げる
|
||||
- Coalesce で実行中追加分が次回に引き継がれる
|
||||
- 空レポートでも新規 Knowledge を作らずに動く（統合は decisions / requests / summary / 既存 Knowledge update のみ）
|
||||
- 整理 phase は git diff で drop / merge / split / trim / rewrite の理由（評価カテゴリ）が読める形で記録される
|
||||
- 置き換えは `status: replaced` + `replaced_by` で残り、直接削除と区別可能
|
||||
|
||||
## 参照
|
||||
|
||||
- `docs/plan/memory.md` §自動化メカニズム / §整理（GC 相当）の扱い / §Compact との関係
|
||||
- `docs/plan/memory-prompts.md` §Phase 2 関連
|
||||
- `tickets/memory-file-format.md`（Linter）
|
||||
- `tickets/memory-search-tools.md`（検索ツール）
|
||||
- `tickets/memory-phase1-extract.md`（staging 生産）
|
||||
- `tickets/memory-usage-metrics.md`（候補レポート / 保護閾値の供給）
|
||||
|
||||
## Review
|
||||
|
||||
- 状態: Approve with follow-up
|
||||
- レビュー詳細: [./memory-phase2-consolidation.review.md](./memory-phase2-consolidation.review.md)
|
||||
- 日付: 2026-05-01
|
||||
|
|
@ -1,88 +0,0 @@
|
|||
# Review: メモリ機構 Phase 2 consolidation + 整理
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
### Trigger
|
||||
- 累積ファイル数 / bytes 閾値で発火: `MemoryConfig::consolidation_threshold_files` / `consolidation_threshold_bytes` を導入し、`try_post_run_consolidate` が両方 `None` なら早期 return、いずれかが満たされれば発火。`crates/pod/src/pod.rs:1533-1537`、`crates/pod/src/pod.rs:1588-1592` で OR 条件評価。要件通り。
|
||||
- compact 同期発火を持たない: `controller.rs` で extract → consolidate → compact の独立順序、compact からの相互呼び出しなし。要件通り。
|
||||
|
||||
### 実行主体と入力
|
||||
- Phase 1 を終えた pod が consolidation Worker を spawn: `try_post_run_extract` → `try_post_run_consolidate` → `try_post_run_compact` の順。`crates/pod/src/controller.rs:397-420`（Run）/ `464-487`（Notify）/ `528-551`（Resume）。要件通り。
|
||||
- 起動時スナップショットで consumed ID list 確定: `run_consolidate_once` が `list_staging_entries` で snapshot を取り、そのまま `consumed_ids` に渡している。`crates/pod/src/pod.rs:1581-1599`。要件通り。
|
||||
- 入力 4 種:
|
||||
- consumed staging エントリ: `render_staging_records` が JSON 整形で全文渡し（`crates/memory/src/consolidate/input.rs:88-101`）。
|
||||
- 既存 `memory/*` 全文: `render_existing_memory_records` で summary / decisions / requests を render（`input.rs:105-123`）。
|
||||
- Knowledge 候補レポート: `KnowledgeCandidateReport::empty()` を渡しており、prompt 上は「(empty — usage metrics pipeline not populated. Do not create new Knowledge records this run.)」と明示（`input.rs:162-166`）。`tickets/memory-usage-metrics.md` 完了までは空入力という ticket §背景 の記述に合致。
|
||||
- 整理材料: `collect_tidy_hints` が Linter Warn 既存ルール（replaced chain / sources overflow / similar slug clusters）と同じ閾値で集計（`crates/memory/src/consolidate/tidy.rs:25-28`、`SOURCES_OVERFLOW_THRESHOLD = 10` / `SIMILAR_SLUG_DISTANCE = 2` を Linter と揃える明示コメントあり）。
|
||||
- 既存 `knowledge/*` 全文を prompt に埋めない: `render_existing_memory_records` は `RecordKind::Knowledge` を弾いている（`input.rs:129`）。`KnowledgeQuery` ツール経由で agent が引く方針と整合。
|
||||
|
||||
### 渡すツール
|
||||
- memory read / write / edit / memory_query / knowledge_query: 全て登録（`crates/pod/src/pod.rs:1643-1647`）。
|
||||
- Linter Hook: memory tool の write/edit が pre-write 検証で `ToolError::InvalidArgument` を返す既存実装にそのまま乗せている。`docs/plan/memory.md` §書き込み経路と Linter の「LLM は通常の tool error フローで違反内容を読み、自己修正する」と整合。
|
||||
- 「汎用 CRUD」: ticket は「file read / write / edit」と書いているが、`docs/plan/memory.md` §書き込み経路と Linter の更新版で memory 専用 Tool 一本化が確定している。本実装は generic ScopedFs CRUD を載せていない（worker は `Worker::new` 直接構築、Pod の Scope/汎用 tool 群を継承しない）ため、結果的に memory tool 経由のみ。これがアーキテクチャ的に正しい選択。
|
||||
|
||||
### 常駐注入の無効化
|
||||
- ticket §常駐注入の無効化 は「Pod 起動直後に `set_resident_knowledge_injection(false)` を呼ぶ」と書いているが、本実装は Pod ではなく `llm_worker::Worker` を直接 spawn する経路（Phase 1 / compact と同一パターン）。
|
||||
- `set_resident_knowledge_injection` は `Pod::prepare_system_prompt` の resident_knowledge 収集分岐（`crates/pod/src/pod.rs:559-576`）にのみ作用し、disposable Worker のシステムプロンプトは `CONSOLIDATION_SYSTEM_PROMPT` を `Worker::new(client).system_prompt(...)` で直接設定（`pod.rs:1619`）。Pod 経路を通らないため、resident-injection は構造的に発生しない。
|
||||
- `pod.rs:1635-1641` のコメントで「Resident knowledge injection (`Pod::set_resident_knowledge_injection`) is a Pod-level concern; this disposable Worker is built without it by construction」と明記。`docs/plan/memory.md` §retrieval 経路 の "Phase 2 prompt には入れない" 規定とも整合。
|
||||
- 結論: ticket 文面と実装が乖離しているが、要件の **意図**（resident injection を載せない / Knowledge 検索ツール経由で引かせる）は構造的に達成されている。ticket 側の記述が古い前提（Pod を立てる想定）に引きずられているだけ。
|
||||
|
||||
### 処理内容（統合 phase / 整理 phase）
|
||||
- prompt: `crates/memory/src/consolidate/prompt.rs` で `docs/plan/memory-prompts.md` §共通原則 / §Phase 2 を縮約。`# Consolidation phase` と `# Tidy phase` の 2 セクション + 共通ルールで「1 セッション内で順に走る」縛りを明示。実装側は単一 sub-Worker 1 run で 2 phase を走らせる構造（独立 trigger なし、`docs/plan/memory.md` §整理 の方針と整合）。
|
||||
- 統合 phase の sources コピー指示、`replaced_by` 運用、knowledge 新規作成 gate（候補レポート空時の作成禁止）、整理 phase の評価カテゴリ 4 種、保護閾値の保守 default 動作 — すべて prompt に明示。
|
||||
- 完了条件「整理 phase が同じ agent セッション内で続けて走る」: prompt 縛りで担保。実装での機械的 2 phase 強制はない（単一 Worker run）。`docs/plan/memory.md` §整理「同じ Worker 同じツール surface で済むため、別 Agent / 別 spawn 経路は設けない」と整合。
|
||||
|
||||
### 並走防止
|
||||
- staging 配下に `_staging/.consolidation.lock`（JSON: `pid` / `pod_name` / `started_at` / `consumed_ids`）。`crates/memory/src/consolidate/lock.rs:23-35`。
|
||||
- in-process は `consolidation_in_flight: AtomicBool` の CAS（`pod.rs:1540-1546`）、cross-process は `kill(pid, 0)` 判定（`lock.rs:170-191`、`pid == 0` と `pid > i32::MAX` を死亡扱い）。2 段防御。
|
||||
- stale lock は `kill` で ESRCH なら上書き取得、JSON parse 失敗も stale 扱い（`lock.rs:97-113`）。
|
||||
- cleanup は consumed ID list のみ削除、追加分は残す（`release_with_cleanup`、`lock.rs:130-147`）。失敗時は `release_only` で staging を保全（`pod.rs:1660-1664`）。要件「実行中に Phase 1 が追加した staging は触らず」を達成。
|
||||
- Coalesce: `try_post_run_consolidate` の loop で `Completed` 後に閾値再評価。Phase 1 の `try_post_run_extract` と同パターン（`pod.rs:1539-1565`）。
|
||||
|
||||
### モデル / 設定
|
||||
- `memory.consolidation_model`（reasoning 系）: `MemoryConfig::consolidation_model` 追加、`build_consolidator_client` で `provider::build_client` 経由構築。`pod.rs:1504-1514`。
|
||||
- `consolidation_worker_max_input_tokens`: default 80,000（`MEMORY_CONSOLIDATION_WORKER_MAX_INPUT_TOKENS`）。compact の 50,000 / extract の 30,000 と比べて妥当な reasoning 寄り上限。
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
- **layer 配置**: 助力モジュール群（prompt / staging / lock / tidy / input）は `crates/memory/src/consolidate/` に集約、spawn 経路（Worker 構築 / interceptor / 在荷フラグ / decision enum）は `crates/pod/src/pod.rs` 内 inline。Phase 1 の `extract` モジュール + `try_post_run_extract` と完全に対称な分離で、低レベル基盤と Pod-level orchestration の境界が明快。`feedback_llm_worker_scope` の指針に整合。
|
||||
- **依存追加**: `cargo add` で memory に `libc = "0.2.186"` / pod に `libc = "0.2.185"` および `uuid` を追加。手動編集ではない（`feedback_cargo_add` 準拠）。ただし libc バージョンが微妙に異なる（後述、Nit）。
|
||||
- **module split**: pod.rs は既に肥大化しているが、本チケット追加分（约 200 行）は `try_post_run_extract` / `run_extract_once` と並行配置で、構造的不整合を増やさない。`MemoryExtractWorkerInterceptor` と並べて `MemoryConsolidationWorkerInterceptor` を separate struct で定義しているのも一貫。
|
||||
- **prompt 配置**: prompt は memory crate 側 (`consolidate/prompt.rs`)。compact prompt は `pod` crate 側 / extract prompt は memory crate 側（`extract::EXTRACT_SYSTEM_PROMPT`）と既存実装で揺れているが、**memory プロンプト系統は memory crate 側に揃える** 方向で extract と統一されており、Phase 2 もそれに従っている。
|
||||
- **テスト分離**: 各モジュール inline `#[cfg(test)]`（lock / staging / tidy / input）+ pod 統合テスト `consolidation_test.rs` の 2 段で、layer ごとに責務切り分けあり。
|
||||
- **過剰実装の有無**: 5 つの helper module は最小構成。daemon 化や DB 化など `docs/plan/memory.md` §将来検討 に列挙された未確定項目は手を出していない。`KnowledgeCandidateReport::empty()` placeholder も usage-metrics チケット完成までの埋め草として最小。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Blocking
|
||||
|
||||
なし。ticket §要件 / §完了条件 の主要項目は満たされ、build / test も全緑。
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
|
||||
- **`Method::PodEvent` 経路のみ consolidate が抜けている**: `crates/pod/src/controller.rs:635-650` は `pod.run_for_notification()` を介して child pod event 由来の auto-kick turn の post-run hook を持つが、この箇所は `try_post_run_extract` → `try_post_run_compact` のみで `try_post_run_consolidate` を呼ばない（他 3 経路 `Method::Run` `Method::Notify` `Method::Resume` は揃っている）。事前討議で「Method::Run と Method::Notify」と表現されたが、Resume が wired されている事実から考えると PodEvent も同様に追加するのが対称的。子 Pod の event だけで生きているような pod が長時間 Phase 2 を踏まなくなるが、次の手動 Run / Notify で吸収される性質のため Blocking ではない。次回 PR で 4 経路統一推奨。
|
||||
- **in-process 重複起動防止のテスト不在**: `consolidation_in_flight: AtomicBool` の CAS は試験コードでは触られていない。`live_lock_held_by_other_pod_skips` は cross-process（lock file）側のみカバー。Phase 1 (`extract_in_flight`) でも同様のギャップなので、本チケットだけの責務ではないが、ticket 完了条件「並走防止ファイルが想定通り機能し、複数 Phase 2 の重複起動が防げる」の **重複起動** 部分は in-process 同時呼び出し（再入）への耐性も含むと読めるため、後追いで concurrent test を 1 件足すと安心。
|
||||
- **Coalesce loop の test 不在**: `Completed` → 再評価 → `Skipped` の round-trip を試すテストがない。`fires_on_threshold_and_cleans_up_consumed_entries` は 1 回 fire の cleanup だけ確認。loop 自体は Phase 1 と同パターンなので回帰確率は低いが、`release_with_cleanup` の後に再 snapshot して staging が残っていれば 2 周目を発火するという ticket §並走防止 の Coalesce 期待値を直接見てはいない。
|
||||
|
||||
### Nits
|
||||
|
||||
- `memory` crate の `libc = "0.2.186"`、`pod` crate の `libc = "0.2.185"` でパッチ番号が揃っていない。Cargo は semver 互換で 0.2.186 に解決するため動作上の問題はないが、いずれか片方を追加した時点でもう一方も合わせると依存ロックが綺麗。
|
||||
- `crates/memory/src/consolidate/lock.rs` の `pid_is_alive` で `kill(0, ...)` / `kill(-1, ...)` を弾く防御コメントが正確で良い。`u32::MAX` を死亡扱いするテスト（`acquire_overwrites_stale_lock`）も合理。
|
||||
- `tidy.rs` の `cluster_similar` で union-find を再実装している。Linter 側の `similar_slugs` と同じアルゴリズムなので、将来は共通化候補（ただし Linter 側はパス引数や報告型が違うため引き剥がしのコストもあり、本チケット範囲では現状維持で問題ない）。
|
||||
|
||||
## 判断
|
||||
|
||||
**Approve with follow-up** — ticket §要件 / §完了条件 の主要項目はすべて satisfied。アーキテクチャは Phase 1 / compact の既存パターンと完全対称で、`docs/plan/memory.md` §自動化メカニズム / §並走防止 / §整理 のセマンティクスを破っていない。`set_resident_knowledge_injection` の ticket 文面 vs 実装の divergence は、コード側コメントで明示されており、結果的に意図（Phase 2 prompt に常駐 injection を載せない）を達成しているため許容範囲。`Method::PodEvent` 経路の consolidate 抜けと、in-process 並走防止 / Coalesce loop の test 不在は次回タスクで詰める価値あり。
|
||||
|
||||
## 参考: 確認した関連箇所
|
||||
|
||||
- `tickets/memory-phase2-consolidation.md`
|
||||
- `docs/plan/memory.md` §自動化メカニズム / §整理（GC 相当）の扱い / §Compact との関係
|
||||
- `docs/plan/memory-prompts.md` §共通原則 / §Phase 2
|
||||
- `crates/memory/src/consolidate/{mod,prompt,staging,lock,tidy,input}.rs`
|
||||
- `crates/memory/src/lib.rs`
|
||||
- `crates/pod/src/pod.rs` （`try_post_run_consolidate` / `run_consolidate_once` / `build_consolidator_client` / `MemoryConsolidationWorkerInterceptor` / `ConsolidateDecision` / `PodError::ConsolidationLock`）
|
||||
- `crates/pod/src/controller.rs` 4 つの post-run 分岐
|
||||
- `crates/pod/tests/consolidation_test.rs`
|
||||
- `crates/manifest/src/{lib,config,defaults}.rs`
|
||||
- `crates/memory/Cargo.toml`、`crates/pod/Cargo.toml`
|
||||
|
|
@ -1,78 +0,0 @@
|
|||
# サブミット入力: TUI 補完 + 型付き atom 化
|
||||
|
||||
## 背景
|
||||
|
||||
`tickets/submit-segment-protocol.md` で protocol が `Vec<Segment>` を運べるようになった前提で、TUI 側に「`@` / `#` / `/` を打鍵中に候補を出し、確定したら typed atom (= 送出時の `Segment`) に昇格させる」UX を載せる。
|
||||
|
||||
`@` / `#` / `/` は TUI の入力アフォーダンスであって protocol contract ではない（GUI などは同じ intent をボタンや picker で表す）。本チケットは TUI 限定の UX に閉じる。
|
||||
|
||||
## 要件
|
||||
|
||||
### token 検出と昇格
|
||||
|
||||
入力中に prefix を検出して候補を浮かせる:
|
||||
|
||||
- `@<部分パス>` — workspace 内のファイル / ディレクトリ候補
|
||||
- `#<部分slug>` — Knowledge slug
|
||||
- `/<部分slug>` — Workflow slug + client-side コマンド（`/clear` など）
|
||||
|
||||
トリガー条件:
|
||||
|
||||
- prefix 記号の直前が「行頭 or 空白 or paste 等の indivisible atom」のときのみ候補ポップアップを開く
|
||||
- 開いた後に prefix 直後がスペースになった時点（例: `@ `, `/ `）で候補を閉じ、通常テキストに戻す
|
||||
|
||||
確定（Tab / Enter 等、入力 UX の詳細は実装で）で対象範囲を `Atom::FileRef` / `Atom::KnowledgeRef` / `Atom::WorkflowInvoke` の indivisible atom に置き換える。挙動は既存の `Atom::Paste` と同等（cursor は中に入れない、Backspace で塊ごと削除）。submit 時に対応する `Segment` 変種に変換して送る。
|
||||
|
||||
### 候補列挙のための protocol query
|
||||
|
||||
補完用に Pod へ問い合わせる軽量経路を追加。`Method::GetHistory` と同じパターンを踏襲する: 専用 `Method` variant を受信した IPC server 層 (`crates/pod/src/ipc/server.rs`) で Controller を介さず直接処理し、対応する `Event` 変種を同ソケットに write back する（broadcast には流さない）。
|
||||
|
||||
扱う query:
|
||||
|
||||
- ファイル候補（Pod scope 内、prefix マッチ）— 本チケットで実装
|
||||
- Knowledge / Workflow slug 候補（kind 指定 + prefix マッチ）— wire の枠だけ用意し、resolver 未登録時は空応答。実体はそれぞれのチケット側
|
||||
|
||||
### Pod 側ファイル resolver（auto-read 切り出し）
|
||||
|
||||
現状 `crates/pod/src/compact/worker.rs` の `mark_read_required` ツールと `crates/pod/src/pod.rs` の再読ロジックに散らばっている auto-read 機構を、「Pod から見たファイルシステム操作」を担う独立モジュール（または新規クレート）に集約する:
|
||||
|
||||
- 既存の auto-read（`ScopedFs` 経由の読み込み + budget 管理）をこのモジュールに移動
|
||||
- 補完候補の prefix マッチ列挙を同モジュールに新設
|
||||
|
||||
このモジュールは Pod の Interceptor / Hook 経路から呼び出される。compact からの利用も新モジュール経由に切り替える。memory / workflow チケットの resolver はここには含めない。
|
||||
|
||||
### 表示
|
||||
|
||||
確定後の atom は paste と同じ「indivisible chip」スタイルで描画する。`@` / `#` / `/` ごとに色を変える程度の差異化を入れる。`Block::UserMessage` 側でも同一スタイルで再描画する（`Event::UserMessage` が typed segment で来る前提）。
|
||||
|
||||
## 範囲外
|
||||
|
||||
- Knowledge / Workflow slug の Pod 側 resolver 実装（それぞれ memory / workflow チケット側で実装。本チケットでは wire の枠と空応答のみ）
|
||||
- 候補スコアリング、fuzzy search、preview 等の高度な補完体験
|
||||
- リッチクライアント（GUI / web）の同等 UX
|
||||
|
||||
## 完了条件
|
||||
|
||||
- `@` / `#` / `/` を打鍵すると候補が出て、確定で chip 化される
|
||||
- chip 化された atom が対応する `Segment` として Pod に送出され、`Event::UserMessage` で戻ってきた typed segment が同じ見た目で再描画される
|
||||
- 候補列挙の query / response が wire を通る
|
||||
- 既存ビルド・テストを壊さない
|
||||
|
||||
## 依存
|
||||
|
||||
- `tickets/submit-segment-protocol.md`
|
||||
|
||||
## 参照
|
||||
|
||||
- `crates/tui/src/input.rs`（`Atom` 体系の拡張）
|
||||
- `crates/tui/src/app.rs`（`submit_input`、`Block::UserMessage` 描画）
|
||||
- `crates/pod/src/ipc/server.rs`（`GetHistory` パターン: Method 受信 → 同ソケットに Event を直接 write back）
|
||||
- `crates/pod/src/compact/worker.rs` / `crates/pod/src/pod.rs`（切り出し対象の auto-read 機構）
|
||||
- `docs/plan/memory.md` §retrieval 経路（slug 補完対象）
|
||||
- `docs/plan/workflow.md` §呼び出しと依存
|
||||
|
||||
## Review
|
||||
|
||||
- 状態: Approve
|
||||
- レビュー詳細: [./submit-tui-completion.review.md](./submit-tui-completion.review.md)
|
||||
- 日付: 2026-04-30
|
||||
|
|
@ -1,48 +0,0 @@
|
|||
# Review: サブミット入力 — TUI 補完 + 型付き atom 化
|
||||
|
||||
レビュー対象: develop ブランチ作業ツリー（未コミット差分含む）。
|
||||
スコープ: チケット `tickets/submit-tui-completion.md` の Phase 1〜4 全体。
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
- **`@` / `#` / `/` 入力中の候補ポップアップ**: `crates/tui/src/input.rs:235-271` の `pending_completion_prefix` がトリガー検出を担い、`crates/tui/src/main.rs:397-417` で popup active 時のキーがオーバーライドされる。要件通り。
|
||||
- **トリガー条件 (sigil 直前 = 行頭/空白/chip atom のみ、sigil 直後スペースで閉じる)**: `pending_completion_prefix` 内 `leading_ok` で判定し、`completion_prefix_tests` (input.rs:794-885) で 9 ケースカバー。`@src/main.rs` の `/` 誤検出回避、`@x ` での閉鎖、`foo@bar` 非トリガー等、要件のエッジを押さえている。
|
||||
- **確定で indivisible chip に置換 (Backspace で塊削除)**: `replace_with_*` (input.rs:200-221) で `drain` + 単一 atom 挿入し cursor を chip 直後に置く。chip atom は `AtomClass::Chip` として既存 word motion / delete_word_before の単位扱いになっており (`atom_class`、`paste_counts_as_one_word` テスト)、Paste と同等の挙動。
|
||||
- **submit 時に対応 `Segment` 変換**: `submit_segments` (input.rs:418-462) が FileRef/KnowledgeRef/WorkflowInvoke を専用 Segment に分けて出す。`knowledge_and_workflow_chips_emit_typed_segments` でカバー。
|
||||
- **候補列挙の protocol query (GetHistory パターン踏襲)**: `Method::ListCompletions` / `Event::Completions` を `crates/protocol/src/lib.rs` に追加、`crates/pod/src/ipc/server.rs:91-118` で同ソケットに直接 reply、broadcast 経路なし。`event_completions_format_and_default_is_dir` / `method_list_completions_roundtrip` で wire 形を固定。
|
||||
- **ファイル resolver = auto-read 切り出し**: `crates/pod/src/fs_view.rs` を新設し、旧 `compact/worker.rs` の `slice_lines` / `ReadRequirement` と旧 `pod.rs` の自動再読ロジックを `PodFsView::render_auto_read` に集約。compact 側は再エクスポート経由で利用 (compact/worker.rs:31)。`Pod::compact` も `PodFsView::new(scoped_fs.clone()).render_auto_read(...)` で同経路を通る (pod.rs:1106)。
|
||||
- **Knowledge / Workflow resolver は wire のみ・空応答**: ipc/server.rs:108-110 で `CompletionKind::Knowledge | Workflow => Vec::new()` と分岐済み。
|
||||
- **chip 表示**: 入力エリアは `Atom::chip()` で全 chip 共通 render (input.rs:83-92)、`Block::UserMessage` は `chip_span_for` で同色 (ui.rs:473-492)。Paste=Magenta, `@`=Cyan, `#`=Green, `/`=Yellow を input area / Block で一致。
|
||||
- **Event::UserMessage の typed segment 経路**: 既存 `submit-segment-protocol.md` が完結済みで、本チケットは Atom 拡張を載せるのみ。
|
||||
|
||||
完了条件 4 項目すべて満たしている。`/clear` の client-side 分岐は仕様変更でチケット側から削除済み。
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
- **`fs_view` モジュールの境界**: 「Pod から見たファイルシステム操作」という単一の責務に絞れており、auto-read と補完列挙が同居しているのは妥当 (どちらも ScopedFs + pwd + scope 上の readable 判定の薄い wrapper)。Knowledge/Workflow resolver は別スロットに将来追加される設計で、`PodFsView` に詰め込む流れにはなっていない (server.rs の kind マッチで分岐済み)。`crates/pod/src/lib.rs` 直下に置く粒度も現状の他モジュール (compact, hook, prompt) と整合。
|
||||
- **`PodSharedState::fs_view: OnceLock<PodFsView>`**: 周辺は `RwLock<T>` 系だが、用途が「controller 起動時に1回 attach、以降 read-only」なので `OnceLock` の方が意図に合う。`set_fs_view` が `set` の戻り値を捨てているのは「unit test が直接生成した state にも噛み合う」コメント通りで適切。Controller 側 (controller.rs:113, 236, 289) で `fs_for_view: tools::ScopedFs;` を宣言してブロック内で代入し、ブロック後に attach する流れも冗長ではない (`fs.clone()` 1回のみ)。
|
||||
- **不必要な抽象化**: 見当たらない。`FileCandidate`, `CompletionEntry` の二重定義は protocol 層と pod 内部層で意図的に分離されており (protocol は wire、pod 内部は path 構造)、`server.rs` の map で繋がっている。
|
||||
- **将来用 dead code**: なし。`CompletionKind::Knowledge | Workflow` は ipc/server.rs で実際にヒットする分岐 (空応答)、`is_dir` フィールドは popup の見た目に使われている。
|
||||
- **コードベースを歪めていないか**: pod 側は単純な抽出、TUI 側は既存 `Atom::Paste` の扱いをそのまま FileRef/KnowledgeRef/WorkflowInvoke に拡張する素直な拡張で、新規の概念導入は最小限。LLM provider policy / cargo add / クレート命名等の方針には影響しない。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
|
||||
- **`is_dir` 候補の確定挙動が popup の hint と乖離している** — `crates/protocol/src/lib.rs` の `CompletionEntry` doc コメント (約 357 行目付近):
|
||||
> `is_dir` is meaningful only for the file kind — it lets the TUI keep a trailing `/` after a directory selection so the user can drill in without re-typing the prefix.
|
||||
と書かれているが、`App::confirm_completion` (`crates/tui/src/app.rs:164-181`) は `entry.value` をそのまま `replace_with_file_ref` に渡すだけで、ディレクトリの場合に `/` を保持して入力継続させる経路がない。popup 側 (`ui.rs:104-110`) は `entry.is_dir` のとき表示に `/` を足しているので、ユーザーには「Tab で drill in できそう」に見えるが実際は chip として閉じる。
|
||||
対応案 (どちらも本チケット範囲外で良い):
|
||||
- doc コメントを実装に合わせて「`is_dir` は popup 表示のヒントのみ」に書き換える
|
||||
- もしくは「ディレクトリ確定時は chip にせず `@subdir/` のテキストを残してカーソルを末尾に置く」挙動を追加する
|
||||
- **Paste 後に `refresh_completion()` が呼ばれない** — `crates/tui/src/main.rs:305-307` の `TermEvent::Paste(s) => app.insert_paste(s);` には refresh 呼び出しがない。`@s` の途中でクリップボード貼付した場合、popup state は古い `(kind, prefix_start, prefix)` のまま残り、次のキーストロークで refresh されるまで 1 フレーム不整合な popup が見える可能性がある。`insert_char` 系と同様に `app.refresh_completion()` を呼んで Method を返す形にしておくのが整合的。
|
||||
- **`compact/worker.rs:358-364` の `slice_lines_handles_offset_and_limit` テストが `fs_view.rs:201-207` と重複** — 関数自体は `fs_view` に移動し compact 側は `use` のみなので、テストもこの移動に合わせて削除して良い (fs_view 側で同一カバレッジが取れている)。
|
||||
|
||||
### Nits
|
||||
|
||||
- `pending_completion_prefix` 内の `self.atoms.get(i.wrapping_sub(1)).filter(|_| i > 0)` (input.rs:253) は意図通り動くが、`if i == 0 { None } else { self.atoms.get(i - 1) }` の方が読みやすい。
|
||||
- `controller.rs:113` の `let fs_for_view: tools::ScopedFs;` 宣言-後代入パターンは正しいが、`fs` 自身を最後に消費しているのを `fs.clone()` を `fs_for_view = fs.clone();` で先に取ってから `register_tools(tools::builtin_tools(fs, ...))` の方が見た目が単純。ただし現状でもコメントで意図が説明されており、可読性の優劣は微差。
|
||||
|
||||
## 判断
|
||||
|
||||
**Approve** — 完了条件 4 項目を満たし、アーキテクチャ・スコープともに歪みなし。フォローアップ事項はいずれも UX 補足やテスト整理で、ブロックすべきものではない。
|
||||
|
|
@ -1,33 +0,0 @@
|
|||
# TUI: マウスホイールでヒストリーをスクロール
|
||||
|
||||
## 背景
|
||||
|
||||
TUIのヒストリービューはキー操作（Shift+↑/↓、PageUp/PageDown、Ctrl+Home/End、Ctrl+[/]）でスクロールできる。スクロール状態（`Scroll` 構造体・follow_tail・top_offset）も既に揃っており、ロジック的には外部から `scroll_up`/`scroll_down` を叩けば任意の手段でスクロール可能になっている。
|
||||
|
||||
一方で、現状のイベントループは `TermEvent::Key` / `Paste` / `Resize` のみを処理しており、マウスイベントは破棄される。crossterm のマウスキャプチャも有効化されていないため、ターミナル側にホイール入力すら届かない。
|
||||
|
||||
エディタ的に常駐するTUIとして、マウスホイールで直感的にヒストリーを遡れる体験を提供したい。
|
||||
|
||||
## 要件
|
||||
|
||||
- フルスクリーンモード中にマウスホイールでヒストリービューをスクロールできる。
|
||||
- ホイール上方向で過去方向（`scroll_up`）、下方向で末尾方向（`scroll_down`）に動く。1 notch あたりの行数は既存のキー操作（Shift+↑/↓）と同じ感覚で良い。
|
||||
- マウスキャプチャの有効化はalt-screenに入っているあいだに限定し、終了時に確実に解除する（picker / spawn のインラインフェーズはネイティブのマウス挙動を保つ）。
|
||||
- クリック・ドラッグ等のホイール以外のマウスイベントは現状無視で構わないが、最低限イベントループでマッチ漏れによるエラーが出ない形にする。
|
||||
|
||||
## 完了条件
|
||||
|
||||
- フルスクリーンTUIでマウスホイールを回すと、ヒストリービューが上下に動く。
|
||||
- TUIを正常終了・異常終了どちらでも、ターミナルのマウスキャプチャ状態が残らない。
|
||||
- 既存のキー操作によるスクロール挙動に変化がない。
|
||||
|
||||
## 範囲外
|
||||
|
||||
- ホイール以外のマウス操作（クリックでフォーカス移動、ドラッグ選択、スクロールバー表示など）
|
||||
- picker / spawn のインラインフェーズでのマウス対応
|
||||
- ホイール感度の設定可能化
|
||||
|
||||
## Review
|
||||
- 状態: Approve
|
||||
- レビュー詳細: [./tui-mouse-scroll.review.md](./tui-mouse-scroll.review.md)
|
||||
- 日付: 2026-05-01
|
||||
|
|
@ -1,40 +0,0 @@
|
|||
# Review: TUI マウスホイールでヒストリーをスクロール
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
- フルスクリーンモード中にマウスホイールでスクロール可能
|
||||
- `enter_fullscreen` で `EnableMouseCapture` を `EnterAlternateScreen` と一緒に発行（`crates/tui/src/main.rs:258`）。`run_loop` の `event::read` 分岐に `TermEvent::Mouse(mouse) => handle_mouse(app, mouse)` を追加（`crates/tui/src/main.rs:314-316`）。要件達成。
|
||||
- ホイール上 = `scroll_up` / 下 = `scroll_down`、感覚は Shift+↑/↓ と同等
|
||||
- `handle_mouse` が `MouseEventKind::ScrollUp/Down` をそのまま `app.scroll.scroll_up/down(WHEEL_LINES)` にマップ（`crates/tui/src/main.rs:366-372`）。`Scroll::scroll_up` は `top_offset` を減らす実装（`scroll.rs:47-50`）で「過去方向」と整合。`WHEEL_LINES = 3` は Shift+↑/↓ の 1 行より速いがページ未満で、ticket の「同じ感覚で良い」と矛盾しない。妥当。
|
||||
- マウスキャプチャは alt-screen 中に限定し、終了時に確実に解除
|
||||
- 有効化は `enter_fullscreen` 1 箇所のみ（`main.rs:258`）。解除は `run_spawn` の child reap 前（`main.rs:237-241`）と `main` 末尾の最終 cleanup（`main.rs:173-178`）の 2 箇所で `DisableMouseCapture` を `LeaveAlternateScreen` と対で発行。picker / spawn のインラインフェーズでは `enter_fullscreen` を経由しないので有効化されない。要件達成。
|
||||
- ホイール以外のマウスイベントでマッチ漏れエラーが出ないこと
|
||||
- `handle_mouse` の `match` は `_ => {}` で吸収（`main.rs:370`）。`run_loop` 側は `TermEvent::Mouse(_)` を新たに拾うので未処理エラーは発生しない。要件達成。
|
||||
|
||||
## 完了条件の検証
|
||||
|
||||
- フルスクリーンTUIでホイールが効く: コードパスは成立。実機確認はユーザー側で実施予定（ticket補足通り）。
|
||||
- 正常終了でターミナル状態が残らない: `main` 末尾の cleanup は `result` の match より前に走るので、`Ok` でも `Err` でも必ず `DisableMouseCapture` を発行する。`run_spawn` 経由で `run` がエラー return しても、戻った直後の `execute!` で disable した後に `?` 相当の伝播が起きるため、二重 disable になっても idempotent で安全。
|
||||
- 既存キー操作のスクロール挙動に変化なし: `Scroll` API・`handle_key` のスクロール経路は無変更。
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
- 変更は `crates/tui/src/main.rs` の terminal lifecycle と event loop に限定。新規モジュール・新規 trait・新規抽象は導入していない。既存の `Scroll` API をそのまま再利用しており、過剰な抽象化はない。
|
||||
- `WHEEL_LINES` を `main.rs` 内の `const` として置いた選択は、現状のキー側のマジックナンバー（page_up/down 等は `area_height` から導出、Shift+↑/↓ は `1` 直書き）と粒度が揃っている。`scroll.rs` 側に押し込む価値は今のところない。
|
||||
- マウス捕捉の対称性（alt-screen と同じスコープでEnable/Disable）は ticket の「範囲外」記述と一致しており、picker / spawn のインラインフェーズでホイール挙動が変わらないことが保証されている。
|
||||
- 依存追加なし。crossterm の既存featureで `MouseEvent` 系が利用可能（既に import 済みの `event` モジュールから引いている）。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
|
||||
- パニック時のクリーンアップは未対応。tui には `std::panic::set_hook` 等のフックが存在せず、これは本変更前から同じ（bracketed paste も alt-screen も同じく panic 時には残る）。本ticket の「異常終了でもキャプチャ状態が残らない」を厳密に取ると未達だが、既存の他リソース（alt-screen、bracketed paste、raw mode）と同レベルで揃っており、本ticketで先んじて対応するのは過剰。別ticket（パニック時のターミナル復帰一括対応）として扱うのが適切。
|
||||
- `enter_fullscreen` 内で `EnterAlternateScreen` の後に `EnableMouseCapture` が失敗した場合、その関数は `?` で早期 return するが、`main` 末尾の cleanup が `DisableMouseCapture, LeaveAlternateScreen` を流すので結果的に状態は復帰する。明示性を上げるなら `enter_fullscreen` 内でロールバックを書く手もあるが、final cleanup と二重になるだけで価値は薄い。現状維持で良い。
|
||||
|
||||
### Nits
|
||||
|
||||
- `WHEEL_LINES` のドキュメントコメントは根拠が読みやすく良い。指摘なし。
|
||||
|
||||
## 判断
|
||||
|
||||
Approve — ticket の前提・要件・完了条件はすべて実装で達成されており、コードベースを歪めず最小差分で収まっている。実機確認はユーザー側で予定通り実施すれば良い。
|
||||
Loading…
Reference in New Issue
Block a user