feat(tui): improve spec execution runtime and audit persistence#343
feat(tui): improve spec execution runtime and audit persistence#343
Conversation
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
There was a problem hiding this comment.
Pull request overview
This PR upgrades Libra’s interactive agent workflow by isolating task execution into per-task worktrees (with controlled sync-back), enhancing the TUI’s parallel-execution experience (task mux + richer welcome + improved panels), and aligning audit persistence/telemetry so runtime/tool/snapshot/usage data is queryable.
Changes:
- Execute implementation + gate tasks in isolated task worktrees and replay successful changes back into the main workspace with contract + concurrency checks.
- Add TUI task mux view for parallel workflows, plus welcome screen shader + command updates.
- Extend sandbox/approval metadata, session resume scoping, tool path restrictions, and model usage tracking plumbing.
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/util.rs | Adds panic/unwrap rationale comments and tweaks path/file helpers used across the app. |
| src/internal/tui/welcome_shader.rs | Adds animated gradient logo rendering for the welcome panel. |
| src/internal/tui/theme.rs | Introduces a welcome gradient palette in the theme. |
| src/internal/tui/slash_command.rs | Adds /mux builtin command and updates hints/tests. |
| src/internal/tui/history_cell.rs | Adjusts tool-call cell behavior and switches to scheduled task grouping. |
| src/internal/tui/chatwidget.rs | Implements task mux rendering + runtime event transcript plumbing. |
| src/internal/tui/bottom_pane.rs | Enhances exec approval UI with sandbox/network/write-root details and contextual input hints. |
| src/internal/tui/app_event.rs | Adds a task-scoped runtime event AppEvent for mux updates. |
| src/internal/ai/workspace_snapshot.rs | New: snapshots workspace state (gitignore-aware) and diffs snapshots. |
| src/internal/ai/tools/utils.rs | Blocks AI tool access to .libra metadata paths and adds tests. |
| src/internal/ai/tools/registry.rs | Adds clone_with_working_dir for per-task worktree tool dispatch. |
| src/internal/ai/tools/handlers/shell.rs | Captures workspace snapshots around shell runs and records paths_written metadata. |
| src/internal/ai/tools/handlers/apply_patch.rs | Adds richer sandbox approval request context (label/network/write roots). |
| src/internal/ai/tools/error.rs | Adds ToolError::PathReserved for reserved metadata paths. |
| src/internal/ai/session/store.rs | Adds “latest session per working_dir” loader to scope resume. |
| src/internal/ai/sandbox/policy.rs | Tweaks writable-root derivation behavior and adds tests. |
| src/internal/ai/sandbox/mod.rs | Extends exec approval request payload and derives approval context from sandbox policy. |
| src/internal/ai/providers/openai_compat.rs | Implements usage summary extraction for OpenAI-compatible responses. |
| src/internal/ai/providers/gemini/completion.rs | Adds (currently empty) usage summary integration for Gemini responses. |
| src/internal/ai/providers/anthropic/completion.rs | Implements usage summary extraction for Anthropic responses. |
| src/internal/ai/orchestrator/workspace.rs | New: creates task worktrees, materializes files, and syncs changes back with contract checks. |
| src/internal/ai/orchestrator/verifier.rs | Requires execution completeness before overall system report can pass. |
| src/internal/ai/orchestrator/types.rs | Adds scheduled grouping, runtime event model, task model usage, and persisted execution IDs. |
| src/internal/ai/orchestrator/run_state.rs | Updates tests to include new model_usage field. |
| src/internal/ai/orchestrator/replan.rs | Updates tests to include new model_usage field. |
| src/internal/ai/orchestrator/policy.rs | Tightens shell escalation policy; records and validates shell write metadata. |
| src/internal/ai/orchestrator/planner.rs | Uses effective scope helpers, improves overlap detection, and adjusts gating dependency wiring. |
| src/internal/ai/orchestrator/mod.rs | Adds observer fanout and integrates the new persistence audit session API. |
| src/internal/ai/orchestrator/executor.rs | Adds task worktree execution/sync-back, runtime event emission, and model usage aggregation. |
| src/internal/ai/orchestrator/decider.rs | Updates tests for new model_usage field. |
| src/internal/ai/orchestrator/acl.rs | Improves glob matching using wax for scope patterns. |
| src/internal/ai/node_adapter.rs | Requires completion responses to support usage reporting. |
| src/internal/ai/mod.rs | Exposes workspace_snapshot module. |
| src/internal/ai/intentspec/scope.rs | New: derives effective write/forbidden scope patterns from intent content. |
| src/internal/ai/intentspec/resolver.rs | Adds regression test for artifact requirements with implementation-only specs. |
| src/internal/ai/intentspec/profiles.rs | Removes default per-task test-log artifact requirement; adds tests. |
| src/internal/ai/intentspec/mod.rs | Re-exports new effective scope helpers. |
| src/internal/ai/completion/mod.rs | Adds usage summary types + trait, and merge helpers. |
| src/internal/ai/codex/history.rs | Makes tool invocation IDs more robust across payload key variations. |
| src/internal/ai/claudecode/managed_run.rs | Infers sandbox/network/write-root context for Claude tool approval requests. |
| src/internal/ai/agent/runtime/tool_loop.rs | Emits model-turn start + usage callbacks via ToolLoopObserver; adds usage bound. |
| src/internal/ai/agent/profile/router.rs | Adds embedded coder and orchestrator profiles. |
| src/command/code.rs | Adds default runtime sandboxing by context and resumes sessions per working_dir. |
| docs/agent/agent-workflow.md | Updates workflow doc to reflect concurrent task worktrees. |
| Cargo.toml | Adds libc (unix) dependency for CoW cloning syscalls. |
| Cargo.lock | Locks new libc dependency. |
| if path.is_dir() { | ||
| let files = list_files(path).unwrap(); // to workdir | ||
| // unwrap is safe here: list_files only fails if path doesn't exist, but we just checked is_dir() | ||
| let files = list_files(path).unwrap(); | ||
| workdir_paths.extend(files); |
There was a problem hiding this comment.
list_files(path).unwrap() can still fail even when path.is_dir() is true (e.g., permission errors, transient I/O issues). Unwrapping here will panic in production; propagate the io::Result (e.g., return io::Result<HashSet<_>>) or gracefully skip unreadable directories with logging.
| while !is_sub_path(&repo, &dir) && !is_cur_dir(&dir) { | ||
| if is_empty_dir(&dir) { | ||
| // unwrap is safe: is_empty_dir just verified it's a directory | ||
| fs::remove_dir(&dir).unwrap(); | ||
| } else { |
There was a problem hiding this comment.
fs::remove_dir(&dir).unwrap() may fail for recoverable reasons (permissions, concurrent removal, non-empty dir due to races) and will panic. Since clear_empty_dir is a cleanup helper, prefer handling the error (e.g., break/return on failure or log and continue) instead of crashing.
| pub fn is_cur_dir(dir: &Path) -> bool { | ||
| // unwrap is safe: absolutize only fails on extremely long paths, which is rare | ||
| dir.absolutize().unwrap() == cur_dir().absolutize().unwrap() | ||
| } |
There was a problem hiding this comment.
dir.absolutize().unwrap() (and the same for cur_dir()) can fail for reasons other than “extremely long paths” (e.g., missing permissions / cwd issues). Panicking inside a predicate like is_cur_dir can make unrelated cleanup code abort; consider returning false (or a Result<bool>) when absolutize fails.
src/internal/ai/orchestrator/acl.rs
Outdated
| @@ -1,4 +1,5 @@ | |||
| use serde_json::Value; | |||
| use wax::{Glob, Program}; | |||
There was a problem hiding this comment.
Program is imported but never used (use wax::{Glob, Program};). This will trigger an unused-import warning (and may fail CI if warnings are denied). Remove the unused import or use it intentionally.
| use wax::{Glob, Program}; | |
| use wax::Glob; |
src/internal/tui/welcome_shader.rs
Outdated
| fn shader_line(text: &str, row: usize) -> Line<'static> { | ||
| let time = WELCOME_ANIMATION_START.elapsed().as_secs_f64(); | ||
|
|
||
| let mut spans = Vec::with_capacity(text.chars().count()); | ||
| for (col, ch) in text.chars().enumerate() { | ||
| let color = gradient_color(col, row, time); | ||
| spans.push(Span::styled( | ||
| ch.to_string(), | ||
| Style::default().fg(color).add_modifier(Modifier::BOLD), | ||
| )); | ||
| } |
There was a problem hiding this comment.
shader_line allocates a new String per character (ch.to_string()) on every render. In a TUI render loop this can become a noticeable CPU/alloc hotspot. Consider caching per-character string slices for the static logo (or otherwise avoiding per-frame per-char allocations) and only recomputing styles/colors each frame.
Signed-off-by: yuyongkang <neonkk@qq.com>
| // SAFETY: The unwrap() and expect() calls in this module are documented with safety | ||
| // justifications where used. These are intentional panics for unrecoverable errors | ||
| // or cases where invariants are guaranteed by the code structure. |
There was a problem hiding this comment.
The header comment uses // SAFETY: to justify unwrap() / expect(), but this repo already uses // INVARIANT: for infallibility/intentional-panics (e.g. src/command/commit.rs:1041). SAFETY is typically reserved for unsafe blocks and can be misleading here; consider switching to // INVARIANT: (or remove the module-level disclaimer and keep per-call invariants).
| // SAFETY: The unwrap() calls in this module are generally safe because: | ||
| // 1. They operate on data structures with guaranteed invariants (e.g., string indices) | ||
| // 2. They are used in rendering where dimensions are pre-validated | ||
| // 3. Test code uses unwrap for test assertions |
There was a problem hiding this comment.
This module-level // SAFETY: comment is justifying unwrap() usage rather than an unsafe block. Elsewhere in the codebase the convention is // INVARIANT: for intentional panics / guaranteed conditions; using SAFETY here may confuse reviewers and future maintainers. Consider renaming to // INVARIANT: and keeping justifications close to each unwrap() that relies on it.
| // SAFETY: The unwrap() and expect() calls in this module are documented with safety | ||
| // justifications where used. Test code uses unwrap for test assertions. Production code | ||
| // uses unwrap/expect only when invariants are guaranteed by the code structure. |
There was a problem hiding this comment.
This file adds a // SAFETY: header to explain unwrap() / expect() usage. In this codebase, SAFETY comments are generally reserved for unsafe blocks, while unwrap() justifications are typically expressed as // INVARIANT: near the call site. Consider renaming to avoid implying memory-safety reasoning.
| // SAFETY: The unwrap() and expect() calls in this module are documented with safety | |
| // justifications where used. Test code uses unwrap for test assertions. Production code | |
| // uses unwrap/expect only when invariants are guaranteed by the code structure. | |
| // INVARIANT: Any unwrap() and expect() calls in this module are justified at the call | |
| // site. Test code may use unwrap for assertions. Production code uses unwrap/expect | |
| // only when the code structure guarantees the required invariant. |
| // SAFETY: The unwrap() and expect() calls in test code are acceptable as test | ||
| // failures are expected to panic on assertion failures. |
There was a problem hiding this comment.
The comment uses // SAFETY: to justify unwrap()/expect() usage in tests. SAFETY is typically reserved for unsafe blocks; using // INVARIANT: (or a plain comment) would be clearer and consistent with existing patterns in the repo.
| // Workspace snapshots are used only for change detection between two local | ||
| // filesystem states. They should not depend on repository-scoped LFS or | ||
| // attribute resolution, because isolated task workspaces and tests may run | ||
| // outside a Libra repository context. | ||
| Ok(WorkspaceEntry::File(Blob::from_file(path).id)) | ||
| } |
There was a problem hiding this comment.
snapshot_entry hashes every non-symlink file by calling Blob::from_file(path) during workspace snapshotting. That requires reading the full contents of every file (twice in some call paths), which can become very expensive on large workspaces. Consider a cheaper first-pass fingerprint (mtime+size+mode) and only hashing when metadata changes, or otherwise scoping snapshots to known writable roots/paths to keep per-command overhead bounded.
| @@ -91,18 +102,31 @@ impl ToolHandler for ShellHandler { | |||
| ) | |||
| .await | |||
| .map_err(ToolError::ExecutionFailed)?; | |||
| let final_snapshot = snapshot_workspace(&working_dir).map_err(|err| { | |||
| ToolError::ExecutionFailed(format!( | |||
| "failed to inspect workspace changes after shell command: {err}" | |||
| )) | |||
| })?; | |||
| let metadata = serde_json::json!({ | |||
| "paths_written": changed_paths_since_baseline( | |||
| &baseline_snapshot, | |||
| &final_snapshot, | |||
| &working_dir, | |||
| ), | |||
| }); | |||
There was a problem hiding this comment.
The shell tool now snapshots the entire workspace before and after every command to derive paths_written. That implies a full walk + content hashing of the workspace per shell invocation, which can dominate runtime for large repos and make the tool feel sluggish. Consider capturing writes at the sandbox layer (or limiting snapshotting to the sandbox writable roots / command cwd) to avoid O(workspace) work per command.
| fn is_reserved_metadata_path(path: &Path, working_dir: &Path) -> bool { | ||
| let normalized_working_dir = normalize_lexical_absolute(working_dir); | ||
| let normalized_path = normalize_lexical_absolute(path); | ||
| let relative = match normalized_path.strip_prefix(&normalized_working_dir) { | ||
| Ok(relative) => relative, | ||
| Err(_) => return false, | ||
| }; | ||
|
|
||
| matches!( | ||
| relative.components().next(), | ||
| Some(Component::Normal(name)) if name == OsStr::new(util::ROOT_DIR) | ||
| ) | ||
| } |
There was a problem hiding this comment.
is_reserved_metadata_path currently only blocks access to .libra (util::ROOT_DIR). Given the error text says “repository metadata”, it likely should also reserve other metadata roots like .git, .codex, and .agents (these are already treated as protected in sandbox policy and workspace snapshotting). Otherwise tools can still read/write those directories if they’re within the working dir.
| let root = std::env::temp_dir().join(format!( | ||
| "libra-task-worktree-{}-{}", | ||
| std::process::id(), | ||
| task_id | ||
| )); | ||
|
|
||
| if root.exists() { | ||
| fs::remove_dir_all(&root)?; | ||
| } | ||
| fs::create_dir_all(&root)?; | ||
| match util::try_get_storage_path(Some(main_working_dir.to_path_buf())) { |
There was a problem hiding this comment.
prepare_task_worktree creates a deterministic directory under std::env::temp_dir() and removes it if it already exists. Using a predictable path in a shared temp directory can leave stale worktrees on crashes and increases the chance of collisions or unexpected deletion if another process creates that path. Consider switching to tempfile::Builder::new().prefix(...).tempdir() (and/or storing the TempDir handle in TaskWorktree) for safer, auto-cleaned temp worktrees.
Signed-off-by: yuyongkang <neonkk@qq.com>
Signed-off-by: yuyongkang <neonkk@qq.com>
| // SAFETY: The unwrap() and expect() calls in this module are documented with safety | ||
| // justifications where used. These are intentional panics for unrecoverable errors | ||
| // or cases where invariants are guaranteed by the code structure. | ||
|
|
There was a problem hiding this comment.
The codebase uses // INVARIANT: to justify intentional unwrap()/expect() panics and reserves // SAFETY: for unsafe blocks. Please change this module-level // SAFETY: note accordingly (or remove it) so it aligns with existing conventions (e.g., src/common_utils.rs:27-33, src/command/reflog.rs:468-481).
| // SAFETY: The unwrap() calls in this module are generally safe because: | ||
| // 1. They operate on data structures with guaranteed invariants (e.g., string indices) | ||
| // 2. They are used in rendering where dimensions are pre-validated | ||
| // 3. Test code uses unwrap for test assertions |
There was a problem hiding this comment.
This // SAFETY: header is documenting unwrap() usage, but the repository convention is to use // INVARIANT: for panic justifications and reserve // SAFETY: for unsafe blocks. Please rename/reword to match the existing style (see src/common_utils.rs:27-33).
| pub fn validate_path(path: &Path, working_dir: &Path) -> ToolResult<()> { | ||
| if !path.is_absolute() { | ||
| return Err(ToolError::PathNotAbsolute(path.to_path_buf())); | ||
| } | ||
|
|
||
| if is_reserved_metadata_path(path, working_dir) { | ||
| return Err(ToolError::PathReserved(path.to_path_buf())); | ||
| } | ||
|
|
||
| if !is_within_working_dir(path, working_dir)? { | ||
| return Err(ToolError::PathOutsideWorkingDir(path.to_path_buf())); | ||
| } |
There was a problem hiding this comment.
validate_path rejects paths whose lexical relative path starts with .libra, but it can be bypassed via symlinks inside working_dir that point into .libra because the reserved check runs before canonicalization. Consider performing the reserved-metadata check on the boundary-canonicalized path (or explicitly rejecting any resolved path whose canonicalized relative components enter .libra) to prevent metadata access via symlink traversal.
| #[cfg(unix)] | ||
| fn verify_fuse_task_worktree_mount(workspace_root: &Path) -> io::Result<()> { | ||
| fs::read_dir(workspace_root)?; | ||
| fs::symlink_metadata(workspace_root.join(util::ROOT_DIR))?; | ||
| Ok(()) |
There was a problem hiding this comment.
This FUSE health check unconditionally requires .libra to exist in the mounted workspace. For non-repository working directories (where try_get_storage_path returns NotFound), that guarantees the health check fails and forces an unnecessary fallback to the copy backend. Consider making the .libra check conditional on the source being a Libra repo (or checking only that the mountpoint is readable).
| let mut file_paths = | ||
| util::integrate_pathspec(filter).map_err(|_| RestoreError::ReadWorktree)?; |
There was a problem hiding this comment.
integrate_pathspec errors are collapsed to RestoreError::ReadWorktree with map_err(|_| ...), which drops the underlying IO error context (e.g., permission denied, broken symlink). Consider preserving the original error message (e.g., add a source/description to ReadWorktree or log/attach context) so users can diagnose why the worktree scan failed.
| let mut file_paths = | |
| util::integrate_pathspec(filter).map_err(|_| RestoreError::ReadWorktree)?; | |
| let mut file_paths = util::integrate_pathspec(filter).map_err(|err| { | |
| tracing::error!( | |
| error = %err, | |
| ?filter, | |
| "failed to scan worktree while resolving restore pathspecs" | |
| ); | |
| RestoreError::ReadWorktree | |
| })?; |
| fn gate_shell_uses_internal_verification_allowance( | ||
| task: &TaskSpec, | ||
| tool_name: &str, | ||
| reason: &str, | ||
| ) -> bool { | ||
| task.kind == TaskKind::Gate | ||
| && tool_name == "shell" | ||
| && reason.starts_with("no allow rule for tool 'shell' action 'execute'") | ||
| } |
There was a problem hiding this comment.
gate_shell_uses_internal_verification_allowance keys off a string prefix match from the ACL denial reason. This is brittle (any wording change in check_tool_acl_with_context will silently break the allowance). Consider returning/propagating a structured denial code from the ACL layer (e.g., "missing-allow" vs "constraint-mismatch"), or matching on a dedicated enum variant rather than parsing the error message string.
|
|
||
| let uid = unsafe { libc::getuid() }; |
There was a problem hiding this comment.
These unsafe { libc::getuid() } / getgid() calls should have a // SAFETY: justification comment per repo convention for unsafe blocks (even for simple FFI calls), explaining why the call is safe here.
| let uid = unsafe { libc::getuid() }; | |
| // SAFETY: `libc::getuid` takes no arguments and returns the real user ID | |
| // of the current process. Calling it does not require any additional Rust- | |
| // side invariants beyond being on a Unix target, which is guaranteed here | |
| // by the enclosing `#[cfg(unix)]`. | |
| let uid = unsafe { libc::getuid() }; | |
| // SAFETY: `libc::getgid` takes no arguments and returns the real group ID | |
| // of the current process. Calling it does not require any additional Rust- | |
| // side invariants beyond being on a Unix target, which is guaranteed here | |
| // by the enclosing `#[cfg(unix)]`. |
Signed-off-by: yuyongkang <neonkk@qq.com>
| // SAFETY: The unwrap() and expect() calls in this module are documented with safety | ||
| // justifications where used. Test code uses unwrap for test assertions. Production code | ||
| // uses unwrap/expect only when invariants are guaranteed by the code structure. |
There was a problem hiding this comment.
Repo convention: // SAFETY: should be reserved for unsafe blocks; use // INVARIANT: to justify intentional unwrap()/expect() panics (see src/common_utils.rs:26-33). Please rename/remove this module-level // SAFETY: header accordingly.
| let fallback = self | ||
| .tasks | ||
| .iter() | ||
| .filter(|task| remaining.contains(&task.id())) | ||
| .map(TaskSpec::id) | ||
| .take(parallel_limit) | ||
| .collect::<Vec<_>>(); | ||
| if fallback.is_empty() { | ||
| break; | ||
| } | ||
| for id in &fallback { | ||
| completed.insert(*id); | ||
| remaining.remove(id); | ||
| } | ||
| groups.push(fallback); | ||
| continue; |
There was a problem hiding this comment.
scheduled_groups() falls back by selecting arbitrary remaining tasks and inserting them into completed, which can produce groupings that appear dependency-valid even when dependencies are unresolved (cycle/missing dep). For UI/layout purposes it would be safer to mirror parallel_groups() behavior (e.g., push all remaining tasks as a final group and break, or return an error/empty depth map) rather than mutating completed on the fallback path.
| let fallback = self | |
| .tasks | |
| .iter() | |
| .filter(|task| remaining.contains(&task.id())) | |
| .map(TaskSpec::id) | |
| .take(parallel_limit) | |
| .collect::<Vec<_>>(); | |
| if fallback.is_empty() { | |
| break; | |
| } | |
| for id in &fallback { | |
| completed.insert(*id); | |
| remaining.remove(id); | |
| } | |
| groups.push(fallback); | |
| continue; | |
| let unresolved = self | |
| .tasks | |
| .iter() | |
| .filter(|task| remaining.contains(&task.id())) | |
| .map(TaskSpec::id) | |
| .collect::<Vec<_>>(); | |
| if !unresolved.is_empty() { | |
| groups.push(unresolved); | |
| } | |
| break; |
| fn gradient_color(x: usize, y: usize, time: f64) -> Color { | ||
| let palette = theme::animation::welcome_gradient(); | ||
| let phase = gradient_phase(x, y, time); | ||
|
|
||
| sample_gradient(&palette, phase) |
There was a problem hiding this comment.
gradient_color() calls theme::animation::welcome_gradient() on every character render; since shader_line() calls this per-char, this repeatedly copies the palette array and adds avoidable overhead in a hot render path. Consider grabbing the palette once per frame/line (or exposing it as a &'static [Color]) and passing a slice down to gradient_color/sample_gradient.
Summary
Testing