mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
 ZakiandClaude
|
b50f80cbda | ||
|
Claude
|
fd6e1d8b1f | ||
|
ZakiandClaude
|
f7fbbc229b |
@@ -1,259 +0,0 @@
|
||||
---
|
||||
name: pr-review-batch
|
||||
description: IronClaw maintainer PR review -- batch review open PRs against ironclaw project standards (Rust, WASM tools, dual-backend DB, security-first)
|
||||
triggers:
|
||||
- review PR
|
||||
- review PRs
|
||||
- review open PRs
|
||||
- batch review
|
||||
- "review #"
|
||||
- check PRs
|
||||
---
|
||||
|
||||
# IronClaw PR Review Workflow
|
||||
|
||||
Maintainer review workflow for the **nearai/ironclaw** repository. Optimized for batch review with parallel data fetching, security-first evaluation against IronClaw's Rust/WASM architecture, and structured GitHub review comments.
|
||||
|
||||
- **Repository:** nearai/ironclaw
|
||||
- **Maintainer GitHub:** zmanian
|
||||
- **Primary language:** Rust (async tokio, wasmtime, axum)
|
||||
- **Key subsystems:** WASM tool sandbox, dual-backend DB (postgres + libsql), LLM provider decorator chain, multi-channel system, SKILL.md skills, registry/installer
|
||||
- **CI jobs that matter:** Formatting, Clippy (default, all-features, libsql-only, Windows), Regression test enforcement
|
||||
- **CI jobs that DON'T prove much:** classify, scope (these always pass, even on fork PRs with no secrets)
|
||||
|
||||
## Review Modes
|
||||
|
||||
The user controls how interactive the review is. Detect the mode from their message:
|
||||
|
||||
| User Says | Mode | Behavior |
|
||||
|-----------|------|----------|
|
||||
| "Review 938, 933" | **Autonomous** | Fetch, evaluate, post reviews without stopping |
|
||||
| "Review PRs. Interview me" | **Interactive** | Present findings, ask for input before posting |
|
||||
| "Check on open PRs" | **Triage** | Summarize state of each PR, ask what to review in depth |
|
||||
| "Approve 683 and 687" | **Direct verdict** | Post the specified verdict without full analysis |
|
||||
|
||||
**Default is autonomous** unless the user says "interview", "ask me", "discuss", "check with me", or similar.
|
||||
|
||||
## Step 1: Parse PR Numbers
|
||||
|
||||
Extract PR numbers from the user's message. Accept formats:
|
||||
- "Review 938, 933, 918"
|
||||
- "Review #834 and #922"
|
||||
- "Review all open PRs" (use `gh pr list --state open --limit 30`)
|
||||
|
||||
## Step 2: Fetch Data (Parallel)
|
||||
|
||||
For EACH PR, fetch all of these in parallel:
|
||||
|
||||
```bash
|
||||
# Metadata: title, author, base/head branch, size
|
||||
gh pr view <N> --json title,author,state,headRefName,baseRefName,additions,deletions,changedFiles \
|
||||
--jq '{title, author: .author.login, state, base: .baseRefName, head: .headRefName, additions, deletions, changedFiles}'
|
||||
|
||||
# Full diff
|
||||
gh pr diff <N> --patch
|
||||
|
||||
# CI status
|
||||
gh pr checks <N>
|
||||
|
||||
# Previous reviews (for re-reviews)
|
||||
gh pr view <N> --json reviews --jq '.reviews[] | {author: .author.login, state: .state, body: .body[:200]}'
|
||||
```
|
||||
|
||||
For large diffs (>1000 lines), use `gh pr diff <N> --patch | head -500` first, then fetch remaining sections as needed. Note Cargo.lock churn separately -- don't count it as meaningful diff.
|
||||
|
||||
## Step 3: Evaluate Each PR
|
||||
|
||||
Check in this priority order:
|
||||
|
||||
### 3a. CI Status
|
||||
- All checks must pass -- not just classify/scope. Must have: Formatting, Clippy (all 3 feature combos), Regression test enforcement.
|
||||
- **Fork PRs (critical gotcha):** Only classify/scope run because GitHub Actions secrets aren't available for fork PRs. The PR will APPEAR to have passing checks. Never trust this. Flag it -- local CI verification or maintainer-triggered re-run required before merge.
|
||||
|
||||
### 3b. Previous Reviews
|
||||
- Check if zmanian already reviewed -- if so, this is a re-review
|
||||
- For re-reviews: verify each previous feedback item was addressed, referencing specific commit hashes
|
||||
- Note reviews from Gemini, Copilot -- cross-reference their findings but don't trust blindly
|
||||
|
||||
### 3c. Security (Highest Priority -- IronClaw-Specific)
|
||||
- **Identity file write protection:** PROTECTED_IDENTITY_FILES (AGENTS.md, SOUL.md, USER.md, IDENTITY.md) must not become LLM-writable
|
||||
- **Tool approval requirements:** ApprovalRequirement changes (Never vs UnlessAutoApproved vs Always) -- especially for tools that cross trust boundaries (tool_install, tool_auth, build_tool, shell)
|
||||
- **WASM sandbox boundaries:** fuel limits, memory limits, network allowlists must not be weakened
|
||||
- **Credential handling:** no secrets in logs/errors/SSE events; use `redact_params()` before broadcast
|
||||
- **SSRF vectors:** URL validation must resolve DNS before checking for private/loopback IPs
|
||||
- **Prompt injection defense:** sanitizer/validator/policy changes in `src/safety/`
|
||||
|
||||
### 3d. Correctness (IronClaw-Specific)
|
||||
- **No `.unwrap()/.expect()` in production code** (tests are fine)
|
||||
- **String safety:** no byte-index slicing (`&s[..n]`) on user/external strings -- use `is_char_boundary()` or `char_indices()`
|
||||
- **Dual-backend DB:** new persistence features must support BOTH postgres AND libsql. Check for missing trait implementations.
|
||||
- **Feature flags:** changes must compile under `--no-default-features --features libsql`, default, and `--all-features`
|
||||
- **Transaction safety:** multi-step DB operations wrapped in transactions (both backends)
|
||||
- **LLM provider decorator chain:** new `LlmProvider` trait methods must be delegated in ALL wrapper types (grep `impl LlmProvider for`)
|
||||
|
||||
### 3e. Architecture & Conventions
|
||||
- `crate::` for cross-module imports (not `super::` except tests and intra-module)
|
||||
- `thiserror` for error types in `error.rs`; map errors with context via `.map_err()`
|
||||
- Strong types over strings (enums, newtypes)
|
||||
- Module specs followed -- if a module has a CLAUDE.md (agent, web, db, llm, setup, tools, workspace), check it
|
||||
- Module-owned initialization: init logic lives in owning module as public factory fn, not in main.rs/app.rs
|
||||
- No unnecessary dependencies (check `~/.claude/approved-dependencies.md` list)
|
||||
|
||||
### 3f. Tests
|
||||
- Bug fixes MUST have regression tests (enforced by CI regression-check job and commit-msg hook)
|
||||
- Tests use `tempfile` crate, not hardcoded `/tmp/` paths
|
||||
- No real network requests in tests (use mocks or RFC 5737 TEST-NET IPs like 192.0.2.1)
|
||||
- Test names and comments match actual test behavior and assertions
|
||||
- `[skip-regression-check]` in commit message or PR label only if genuinely not feasible
|
||||
|
||||
## Step 4: Interview (Interactive Mode)
|
||||
|
||||
In interactive mode, present findings and ask for the maintainer's judgment before posting. **Do NOT post reviews until the maintainer confirms.**
|
||||
|
||||
### When to Interview (Even in Autonomous Mode)
|
||||
|
||||
Always pause and ask the maintainer when you encounter:
|
||||
|
||||
1. **Judgment calls on architecture direction** -- "This PR adds a named provider for Z.AI. Should we prefer named providers or push contributors toward openai_compatible for niche providers?"
|
||||
2. **Security tradeoffs with usability** -- "Removing approval from tool_install reduces friction but weakens the trust boundary. What's your stance?"
|
||||
3. **Scope creep concerns** -- "This PR started as a bug fix but adds 300 lines of new feature. Accept as-is or ask to split?"
|
||||
4. **Dependency additions** -- "This adds `datafusion` (heavy dep). Worth it for the use case?"
|
||||
5. **Contradictory signals** -- "Gemini approved but Copilot flagged a real issue. The code works but the pattern is fragile."
|
||||
6. **Taking over vs requesting changes** -- "This PR has 5+ issues. Want me to take it over or send detailed feedback?"
|
||||
7. **Merge ordering for conflicting PRs** -- "PRs #933 and #918 both modify cli/mod.rs. Which should land first?"
|
||||
|
||||
### Interview Format
|
||||
|
||||
Present findings concisely, then ask a specific question:
|
||||
|
||||
```
|
||||
**PR #922: Relax tool approval requirements**
|
||||
|
||||
The HTTP GET change is clean (tiered: credentials->Always, GET->Never, other->UnlessAutoApproved).
|
||||
|
||||
But it also removes approval from:
|
||||
- build_tool (can execute shell commands)
|
||||
- tool_install (downloads WASM modules)
|
||||
- tool_auth (grants credentials to tools)
|
||||
|
||||
These cross the trust boundary. Options:
|
||||
1. Approve as-is (maximum convenience)
|
||||
2. Request changes: keep build_tool + extension tools gated, accept the rest
|
||||
3. Request changes: revert everything except HTTP GET and list_dir
|
||||
|
||||
Which direction?
|
||||
```
|
||||
|
||||
Wait for the maintainer's response before posting.
|
||||
|
||||
### Triage Mode
|
||||
|
||||
In triage mode, present a dashboard first:
|
||||
|
||||
```
|
||||
| PR | Author | Title | CI | Reviews | Age | Risk |
|
||||
|----|--------|-------|----|---------|-----|------|
|
||||
| #938 | reidliu41 | Z.AI provider | green | none | 1d | low |
|
||||
| #922 | ilblackdragon | relax approvals | green | copilot:concern | 2d | medium |
|
||||
| #927 | ilblackdragon | chat onboarding | green | zmanian:changes | 3d | high |
|
||||
```
|
||||
|
||||
Then ask: "Which ones should I review in depth? Or should I go through all of them?"
|
||||
|
||||
## Step 5: Determine Verdict
|
||||
|
||||
| Verdict | Criteria |
|
||||
|---------|----------|
|
||||
| **APPROVE** | Clean, follows IronClaw patterns, full CI green, no security issues, tests present |
|
||||
| **REQUEST CHANGES** | Security regressions, functional bugs, .expect() in production, trust boundary violations, missing dual-backend support, missing error handling |
|
||||
| **COMMENT** | Good direction but needs discussion, or already approved with observations |
|
||||
|
||||
In interactive mode, confirm the verdict with the maintainer before posting. In autonomous mode, post directly.
|
||||
|
||||
## Step 6: Post Reviews
|
||||
|
||||
Post reviews via `gh pr review` using HEREDOC for body formatting.
|
||||
|
||||
### New Review Format
|
||||
|
||||
```
|
||||
## Review: <short summary of what PR does>
|
||||
|
||||
<1-2 sentence assessment>
|
||||
|
||||
Positives:
|
||||
- <what works well>
|
||||
- <pattern compliance>
|
||||
|
||||
### <Severity>: <issue title>
|
||||
<Detailed explanation>
|
||||
|
||||
### <Severity>: <issue title>
|
||||
<Detailed explanation>
|
||||
|
||||
Minor notes:
|
||||
- <non-blocking observation>
|
||||
|
||||
<Concrete suggestion if requesting changes>
|
||||
```
|
||||
|
||||
Severity levels: Critical, Concerning, Minor (non-blocking)
|
||||
|
||||
### Re-Review Format
|
||||
|
||||
```
|
||||
## Re-review: <status summary>
|
||||
|
||||
All/N items from my previous review have been resolved:
|
||||
|
||||
1. **<item>** -- Fixed in commit <hash>. <What changed>.
|
||||
2. **<item>** -- Fixed. <Details>.
|
||||
|
||||
<Additional observations if any>
|
||||
|
||||
LGTM.
|
||||
```
|
||||
|
||||
## Step 7: Handle GitHub API Errors
|
||||
|
||||
GitHub 502s are common during batch posting. Retry with `sleep 5` between attempts. Post reviews sequentially (not in parallel) to avoid rate limits.
|
||||
|
||||
## Step 8: Summary
|
||||
|
||||
After all reviews are posted, provide a summary table:
|
||||
|
||||
```
|
||||
| PR | Title | Verdict |
|
||||
|----|-------|---------|
|
||||
| #938 | Z.AI provider | Approved |
|
||||
| #933 | channels list CLI | Approved |
|
||||
| #918 | skills CLI | Approved |
|
||||
```
|
||||
|
||||
Note cross-PR conflicts (e.g., PRs that both modify `src/cli/mod.rs` and snapshot files).
|
||||
|
||||
## Special Cases
|
||||
|
||||
### Fork PRs
|
||||
Only classify/scope CI jobs run. **Never merge with only these passing.** Either:
|
||||
- Run local CI: `cargo check --all-features && cargo clippy --all && cargo test`
|
||||
- Or trigger full CI by pushing a maintainer commit to the PR branch
|
||||
|
||||
### Registry/WASM PRs
|
||||
- Verify artifact URLs match the naming convention: `<kind>-<name>-<version>-wasm32-wasip2.tar.gz`
|
||||
- Check SHA256 checksums against actual release assets
|
||||
- Ensure `name` field in manifest matches crate_name in source config
|
||||
- Cross-reference with `.github/workflows/release.yml` for automated patching
|
||||
|
||||
### Taking Over a PR
|
||||
When a contributor PR has too many issues:
|
||||
1. Create new branch from staging
|
||||
2. Cherry-pick or apply the contributor's changes
|
||||
3. Fix the issues
|
||||
4. Create superseding PR referencing the original
|
||||
|
||||
### Cross-PR Context
|
||||
When PRs are related (e.g., all touch registry manifests, or both modify cli/mod.rs), post context comments on each explaining how they fit together and merge ordering.
|
||||
|
||||
### Batch Merge
|
||||
When the user says "merge" after reviews, use `gh pr merge <N> --squash` for each approved PR. Verify CI is still green before each merge.
|
||||
@@ -1,4 +0,0 @@
|
||||
{
|
||||
"setup": [],
|
||||
"teardown": []
|
||||
}
|
||||
@@ -12,9 +12,6 @@
|
||||
<a href="#license"><img src="https://img.shields.io/badge/license-MIT%20OR%20Apache%202.0-blue.svg" alt="License: MIT OR Apache-2.0" /></a>
|
||||
<a href="https://t.me/ironclawAI"><img src="https://img.shields.io/badge/Telegram-%40ironclawAI-26A5E4?style=flat&logo=telegram&logoColor=white" alt="Telegram: @ironclawAI" /></a>
|
||||
<a href="https://www.reddit.com/r/ironclawAI/"><img src="https://img.shields.io/badge/Reddit-r%2FironclawAI-FF4500?style=flat&logo=reddit&logoColor=white" alt="Reddit: r/ironclawAI" /></a>
|
||||
<a href="https://gitcgr.com/nearai/ironclaw">
|
||||
<img src="https://gitcgr.com/badge/nearai/ironclaw.svg" alt="gitcgr" />
|
||||
</a>
|
||||
</p>
|
||||
|
||||
<p align="center">
|
||||
|
||||
@@ -1,101 +0,0 @@
|
||||
# Proactive Docker Detection
|
||||
|
||||
Date: 2026-02-21
|
||||
|
||||
## Problem
|
||||
|
||||
IronClaw's sandbox system requires Docker but provides no proactive guidance. Docker availability is only checked at runtime when a sandbox job is attempted, resulting in a confusing error. Users have no way to know during setup or startup whether Docker is properly configured.
|
||||
|
||||
## Goals
|
||||
|
||||
1. Detect Docker installation AND daemon running status at two points: setup wizard and every startup
|
||||
2. Provide platform-specific installation guidance (macOS, Linux, Windows)
|
||||
3. Surface Docker status clearly in the boot screen
|
||||
4. Allow users to skip/continue without Docker (sandbox is optional)
|
||||
|
||||
## Non-Goals
|
||||
|
||||
- Auto-installing Docker
|
||||
- Changing the default sandbox setting (stays `enabled: false`)
|
||||
- Modifying the existing `connect_docker()` function
|
||||
|
||||
## Design
|
||||
|
||||
### Docker Status Model
|
||||
|
||||
New file `src/sandbox/detect.rs` with centralized detection:
|
||||
|
||||
```rust
|
||||
pub enum DockerStatus {
|
||||
Available, // Binary on PATH + daemon responding to ping
|
||||
NotInstalled, // `docker` binary not found on PATH
|
||||
NotRunning, // Binary found but daemon not responding
|
||||
Disabled, // Sandbox not enabled (no check performed)
|
||||
}
|
||||
|
||||
pub enum Platform { MacOS, Linux, Windows }
|
||||
|
||||
pub struct DockerDetection {
|
||||
pub status: DockerStatus,
|
||||
pub platform: Platform,
|
||||
}
|
||||
```
|
||||
|
||||
Detection logic:
|
||||
1. Check if `docker` binary exists on PATH (reuse `which`/`where` pattern from `skills/gating.rs`)
|
||||
2. If found, attempt `connect_docker()` to ping the daemon
|
||||
3. Return `Available`, `NotInstalled`, or `NotRunning`
|
||||
|
||||
### Platform-Specific Guidance
|
||||
|
||||
| Platform | Not Installed | Not Running |
|
||||
|----------|--------------|-------------|
|
||||
| macOS | "Install Docker Desktop: https://docs.docker.com/desktop/install/mac-install/" | "Start Docker Desktop from Applications, or run: open -a Docker" |
|
||||
| Linux | "Install Docker Engine: https://docs.docker.com/engine/install/" | "Start the Docker daemon: sudo systemctl start docker" |
|
||||
| Windows | "Install Docker Desktop: https://docs.docker.com/desktop/install/windows-install/" | "Start Docker Desktop from the Start menu" |
|
||||
|
||||
### Wizard Step (First-Run)
|
||||
|
||||
Add Step 8 "Docker Sandbox" (current steps 8 becomes 9, total becomes 9):
|
||||
|
||||
1. Ask "Do you want to enable Docker sandbox for isolated code execution?"
|
||||
2. If yes, run Docker detection
|
||||
3. Based on status:
|
||||
- **Available**: Enable sandbox, confirm
|
||||
- **Not Installed**: Show install guidance, offer to skip or retry after installing
|
||||
- **Not Running**: Show start guidance, offer to skip or retry
|
||||
4. If user skips, sandbox stays disabled
|
||||
|
||||
### Startup Check (Every Launch)
|
||||
|
||||
In `main.rs`, when `config.sandbox.enabled == true`, before creating `ContainerJobManager`:
|
||||
|
||||
1. Run `DockerDetection::check()`
|
||||
2. If **Available**: proceed normally
|
||||
3. If **NotInstalled** or **NotRunning**: log warning, disable sandbox for this session, continue startup
|
||||
|
||||
### Boot Screen Changes
|
||||
|
||||
`BootInfo` gains `docker_status: DockerStatus` field.
|
||||
|
||||
Features line rendering:
|
||||
- `Available` + enabled: `sandbox` (as today)
|
||||
- `NotInstalled` + enabled in config: `sandbox (docker not installed)`
|
||||
- `NotRunning` + enabled in config: `sandbox (docker not running)`
|
||||
- `Disabled`: no sandbox shown (as today)
|
||||
|
||||
Warning lines shown in yellow when Docker is configured but unavailable.
|
||||
|
||||
## Files
|
||||
|
||||
| Action | File | Change |
|
||||
|--------|------|--------|
|
||||
| Create | `src/sandbox/detect.rs` | Detection logic, platform hints |
|
||||
| Modify | `src/sandbox/mod.rs` | Export `detect` module |
|
||||
| Modify | `src/setup/wizard.rs` | Add Docker/Sandbox wizard step |
|
||||
| Modify | `src/main.rs` | Startup check before ContainerJobManager |
|
||||
| Modify | `src/boot_screen.rs` | Show Docker status |
|
||||
|
||||
## Dependencies
|
||||
|
||||
No new crate dependencies. Uses existing `bollard` (via `connect_docker()`), `std::process::Command` (for binary detection), and `std::env::consts::OS` (for platform detection).
|
||||
@@ -1,450 +0,0 @@
|
||||
# Docker Detection Implementation Plan
|
||||
|
||||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.
|
||||
|
||||
**Goal:** Add proactive Docker detection at startup and in the setup wizard, with platform-specific installation guidance.
|
||||
|
||||
**Architecture:** New `src/sandbox/detect.rs` module for centralized Docker detection. Wizard gets a new step. Startup check in `main.rs` warns and disables sandbox if Docker unavailable. Boot screen shows Docker status.
|
||||
|
||||
**Tech Stack:** Rust, bollard (existing), std::process::Command
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Create `src/sandbox/detect.rs` -- Docker Detection Module
|
||||
|
||||
**Files:**
|
||||
- Create: `src/sandbox/detect.rs`
|
||||
- Modify: `src/sandbox/mod.rs`
|
||||
|
||||
**Step 1: Write the failing test**
|
||||
|
||||
```rust
|
||||
// In src/sandbox/detect.rs
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn test_detect_platform() {
|
||||
let platform = Platform::current();
|
||||
// Should return a valid platform on any CI/dev machine
|
||||
match platform {
|
||||
Platform::MacOS | Platform::Linux | Platform::Windows => {}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_install_hint_not_empty() {
|
||||
for platform in [Platform::MacOS, Platform::Linux, Platform::Windows] {
|
||||
assert!(!platform.install_hint().is_empty());
|
||||
assert!(!platform.start_hint().is_empty());
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_docker_status_display() {
|
||||
assert_eq!(DockerStatus::Available.as_str(), "available");
|
||||
assert_eq!(DockerStatus::NotInstalled.as_str(), "not installed");
|
||||
assert_eq!(DockerStatus::NotRunning.as_str(), "not running");
|
||||
assert_eq!(DockerStatus::Disabled.as_str(), "disabled");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_docker_status_is_ok() {
|
||||
assert!(DockerStatus::Available.is_ok());
|
||||
assert!(!DockerStatus::NotInstalled.is_ok());
|
||||
assert!(!DockerStatus::NotRunning.is_ok());
|
||||
assert!(!DockerStatus::Disabled.is_ok());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_check_docker_returns_valid_status() {
|
||||
let result = check_docker().await;
|
||||
// On CI without Docker, should be NotInstalled or NotRunning
|
||||
// On dev with Docker, should be Available
|
||||
// Either way, should not panic
|
||||
match result.status {
|
||||
DockerStatus::Available
|
||||
| DockerStatus::NotInstalled
|
||||
| DockerStatus::NotRunning => {}
|
||||
DockerStatus::Disabled => panic!("check_docker should never return Disabled"),
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Step 2: Write the implementation**
|
||||
|
||||
```rust
|
||||
//! Proactive Docker detection with platform-specific guidance.
|
||||
|
||||
/// Docker daemon availability status.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum DockerStatus {
|
||||
/// Docker binary found on PATH and daemon responding to ping.
|
||||
Available,
|
||||
/// `docker` binary not found on PATH.
|
||||
NotInstalled,
|
||||
/// Binary found but daemon not responding.
|
||||
NotRunning,
|
||||
/// Sandbox feature not enabled (no check performed).
|
||||
Disabled,
|
||||
}
|
||||
|
||||
impl DockerStatus {
|
||||
pub fn is_ok(&self) -> bool {
|
||||
matches!(self, DockerStatus::Available)
|
||||
}
|
||||
|
||||
pub fn as_str(&self) -> &'static str {
|
||||
match self {
|
||||
DockerStatus::Available => "available",
|
||||
DockerStatus::NotInstalled => "not installed",
|
||||
DockerStatus::NotRunning => "not running",
|
||||
DockerStatus::Disabled => "disabled",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Host platform for install guidance.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum Platform {
|
||||
MacOS,
|
||||
Linux,
|
||||
Windows,
|
||||
}
|
||||
|
||||
impl Platform {
|
||||
pub fn current() -> Self {
|
||||
match std::env::consts::OS {
|
||||
"macos" => Platform::MacOS,
|
||||
"windows" => Platform::Windows,
|
||||
_ => Platform::Linux,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn install_hint(&self) -> &'static str {
|
||||
match self {
|
||||
Platform::MacOS => "Install Docker Desktop: https://docs.docker.com/desktop/install/mac-install/",
|
||||
Platform::Linux => "Install Docker Engine: https://docs.docker.com/engine/install/",
|
||||
Platform::Windows => "Install Docker Desktop: https://docs.docker.com/desktop/install/windows-install/",
|
||||
}
|
||||
}
|
||||
|
||||
pub fn start_hint(&self) -> &'static str {
|
||||
match self {
|
||||
Platform::MacOS => "Start Docker Desktop from Applications, or run: open -a Docker",
|
||||
Platform::Linux => "Start the Docker daemon: sudo systemctl start docker",
|
||||
Platform::Windows => "Start Docker Desktop from the Start menu",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Result of a Docker detection check.
|
||||
pub struct DockerDetection {
|
||||
pub status: DockerStatus,
|
||||
pub platform: Platform,
|
||||
}
|
||||
|
||||
/// Check whether Docker is installed and running.
|
||||
///
|
||||
/// 1. Checks if `docker` binary exists on PATH
|
||||
/// 2. If found, tries to connect and ping the Docker daemon
|
||||
/// 3. Returns `Available`, `NotInstalled`, or `NotRunning`
|
||||
pub async fn check_docker() -> DockerDetection {
|
||||
let platform = Platform::current();
|
||||
|
||||
// Step 1: Check if docker binary is on PATH
|
||||
if !docker_binary_exists() {
|
||||
return DockerDetection {
|
||||
status: DockerStatus::NotInstalled,
|
||||
platform,
|
||||
};
|
||||
}
|
||||
|
||||
// Step 2: Try to connect to the daemon
|
||||
match crate::sandbox::connect_docker().await {
|
||||
Ok(_) => DockerDetection {
|
||||
status: DockerStatus::Available,
|
||||
platform,
|
||||
},
|
||||
Err(_) => DockerDetection {
|
||||
status: DockerStatus::NotRunning,
|
||||
platform,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
/// Check if the `docker` binary exists on PATH.
|
||||
fn docker_binary_exists() -> bool {
|
||||
#[cfg(unix)]
|
||||
{
|
||||
std::process::Command::new("which")
|
||||
.arg("docker")
|
||||
.stdout(std::process::Stdio::null())
|
||||
.stderr(std::process::Stdio::null())
|
||||
.status()
|
||||
.is_ok_and(|s| s.success())
|
||||
}
|
||||
#[cfg(windows)]
|
||||
{
|
||||
std::process::Command::new("where")
|
||||
.arg("docker")
|
||||
.stdout(std::process::Stdio::null())
|
||||
.stderr(std::process::Stdio::null())
|
||||
.status()
|
||||
.is_ok_and(|s| s.success())
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Step 3: Export from `src/sandbox/mod.rs`**
|
||||
|
||||
Add `pub mod detect;` and re-export key types.
|
||||
|
||||
**Step 4: Run tests**
|
||||
|
||||
Run: `cargo test sandbox::detect::tests -- --nocapture`
|
||||
Expected: All pass
|
||||
|
||||
**Step 5: Clippy**
|
||||
|
||||
Run: `cargo clippy --all --all-features`
|
||||
Expected: Zero warnings on new code
|
||||
|
||||
**Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add src/sandbox/detect.rs src/sandbox/mod.rs
|
||||
git commit -m "feat: add Docker detection module with platform guidance"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Update Boot Screen to Show Docker Status
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/boot_screen.rs`
|
||||
|
||||
**Step 1: Add `docker_status` to `BootInfo`**
|
||||
|
||||
Add field: `pub docker_status: DockerStatus` (import from `crate::sandbox::detect::DockerStatus`).
|
||||
|
||||
**Step 2: Update `print_boot_screen` features rendering**
|
||||
|
||||
When sandbox is enabled in config but Docker isn't available, show a warning:
|
||||
- `DockerStatus::Available`: "sandbox" (as today)
|
||||
- `DockerStatus::NotInstalled`: "sandbox (docker not installed)" in yellow
|
||||
- `DockerStatus::NotRunning`: "sandbox (docker not running)" in yellow
|
||||
- `DockerStatus::Disabled`: don't show sandbox (as today)
|
||||
|
||||
**Step 3: Update tests**
|
||||
|
||||
Update all 3 existing `BootInfo` test structs to include `docker_status` field.
|
||||
|
||||
**Step 4: Run tests**
|
||||
|
||||
Run: `cargo test boot_screen::tests`
|
||||
Expected: All pass
|
||||
|
||||
**Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add src/boot_screen.rs
|
||||
git commit -m "feat: show Docker status in boot screen"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Add Startup Docker Check in `main.rs`
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/main.rs`
|
||||
|
||||
**Step 1: Add Docker check before `ContainerJobManager` creation**
|
||||
|
||||
Before line ~989 (`let container_job_manager = if config.sandbox.enabled`), insert:
|
||||
|
||||
```rust
|
||||
// Proactive Docker detection
|
||||
let docker_status = if config.sandbox.enabled {
|
||||
let detection = ironclaw::sandbox::detect::check_docker().await;
|
||||
match detection.status {
|
||||
ironclaw::sandbox::detect::DockerStatus::Available => {
|
||||
tracing::info!("Docker is available");
|
||||
detection.status
|
||||
}
|
||||
ironclaw::sandbox::detect::DockerStatus::NotInstalled => {
|
||||
tracing::warn!(
|
||||
"Docker is not installed. Sandbox disabled for this session. {}",
|
||||
detection.platform.install_hint()
|
||||
);
|
||||
detection.status
|
||||
}
|
||||
ironclaw::sandbox::detect::DockerStatus::NotRunning => {
|
||||
tracing::warn!(
|
||||
"Docker is installed but not running. Sandbox disabled for this session. {}",
|
||||
detection.platform.start_hint()
|
||||
);
|
||||
detection.status
|
||||
}
|
||||
ironclaw::sandbox::detect::DockerStatus::Disabled => detection.status,
|
||||
}
|
||||
} else {
|
||||
ironclaw::sandbox::detect::DockerStatus::Disabled
|
||||
};
|
||||
```
|
||||
|
||||
Then gate the `ContainerJobManager` creation on `docker_status.is_ok()`:
|
||||
```rust
|
||||
let container_job_manager = if config.sandbox.enabled && docker_status.is_ok() {
|
||||
// ... existing code ...
|
||||
```
|
||||
|
||||
**Step 2: Pass `docker_status` to `BootInfo`**
|
||||
|
||||
In the boot screen construction, add the `docker_status` field.
|
||||
|
||||
**Step 3: Run full test suite**
|
||||
|
||||
Run: `cargo test`
|
||||
Expected: All pass
|
||||
|
||||
**Step 4: Commit**
|
||||
|
||||
```bash
|
||||
git add src/main.rs
|
||||
git commit -m "feat: check Docker availability at startup"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Add Docker/Sandbox Wizard Step
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/setup/wizard.rs`
|
||||
|
||||
**Step 1: Increment `total_steps` from 8 to 9**
|
||||
|
||||
**Step 2: Add `step_docker_sandbox()` method**
|
||||
|
||||
Insert after Extensions (step 7), before Heartbeat:
|
||||
|
||||
```rust
|
||||
/// Step 8: Docker Sandbox
|
||||
async fn step_docker_sandbox(&mut self) -> Result<(), SetupError> {
|
||||
print_info("The Docker sandbox provides isolated execution for code generation,");
|
||||
print_info("builds, and untrusted commands. It requires Docker to be installed.");
|
||||
println!();
|
||||
|
||||
if !confirm("Enable Docker sandbox?", false).map_err(SetupError::Io)? {
|
||||
self.settings.sandbox.enabled = false;
|
||||
print_info("Sandbox disabled. You can enable it later with SANDBOX_ENABLED=true.");
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
// Check Docker availability
|
||||
let detection = crate::sandbox::detect::check_docker().await;
|
||||
|
||||
match detection.status {
|
||||
crate::sandbox::detect::DockerStatus::Available => {
|
||||
self.settings.sandbox.enabled = true;
|
||||
print_success("Docker is installed and running. Sandbox enabled.");
|
||||
}
|
||||
crate::sandbox::detect::DockerStatus::NotInstalled => {
|
||||
println!();
|
||||
print_error("Docker is not installed.");
|
||||
print_info(detection.platform.install_hint());
|
||||
println!();
|
||||
// Offer retry or skip
|
||||
if confirm("Retry after installing Docker?", false).map_err(SetupError::Io)? {
|
||||
let retry = crate::sandbox::detect::check_docker().await;
|
||||
if retry.status.is_ok() {
|
||||
self.settings.sandbox.enabled = true;
|
||||
print_success("Docker is now available. Sandbox enabled.");
|
||||
} else {
|
||||
self.settings.sandbox.enabled = false;
|
||||
print_info("Docker still not available. Sandbox disabled for now.");
|
||||
}
|
||||
} else {
|
||||
self.settings.sandbox.enabled = false;
|
||||
print_info("Sandbox disabled. Install Docker and set SANDBOX_ENABLED=true later.");
|
||||
}
|
||||
}
|
||||
crate::sandbox::detect::DockerStatus::NotRunning => {
|
||||
println!();
|
||||
print_error("Docker is installed but not running.");
|
||||
print_info(detection.platform.start_hint());
|
||||
println!();
|
||||
if confirm("Retry after starting Docker?", false).map_err(SetupError::Io)? {
|
||||
let retry = crate::sandbox::detect::check_docker().await;
|
||||
if retry.status.is_ok() {
|
||||
self.settings.sandbox.enabled = true;
|
||||
print_success("Docker is now running. Sandbox enabled.");
|
||||
} else {
|
||||
self.settings.sandbox.enabled = false;
|
||||
print_info("Docker still not responding. Sandbox disabled for now.");
|
||||
}
|
||||
} else {
|
||||
self.settings.sandbox.enabled = false;
|
||||
print_info("Sandbox disabled. Start Docker and set SANDBOX_ENABLED=true later.");
|
||||
}
|
||||
}
|
||||
_ => {
|
||||
self.settings.sandbox.enabled = false;
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
```
|
||||
|
||||
**Step 3: Wire into `run()` method**
|
||||
|
||||
```rust
|
||||
// Step 8: Docker Sandbox
|
||||
print_step(8, total_steps, "Docker Sandbox");
|
||||
self.step_docker_sandbox().await?;
|
||||
self.persist_after_step().await;
|
||||
|
||||
// Step 9: Heartbeat (was Step 8)
|
||||
print_step(9, total_steps, "Background Tasks");
|
||||
self.step_heartbeat()?;
|
||||
self.persist_after_step().await;
|
||||
```
|
||||
|
||||
**Step 4: Run tests**
|
||||
|
||||
Run: `cargo test setup`
|
||||
Expected: All pass
|
||||
|
||||
**Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add src/setup/wizard.rs
|
||||
git commit -m "feat: add Docker sandbox step to setup wizard"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 5: Final Verification
|
||||
|
||||
**Step 1: Run full test suite**
|
||||
|
||||
Run: `cargo test`
|
||||
Expected: All pass
|
||||
|
||||
**Step 2: Run clippy**
|
||||
|
||||
Run: `cargo clippy --all --all-features --benches --tests --examples`
|
||||
Expected: Zero warnings
|
||||
|
||||
**Step 3: Check for unwrap/expect in production code**
|
||||
|
||||
Grep changed files for `.unwrap()` and `.expect(` -- should have none in production code.
|
||||
|
||||
**Step 4: Verify both feature flags compile**
|
||||
|
||||
Run: `cargo check` and `cargo check --no-default-features --features libsql`
|
||||
Expected: Both clean
|
||||
@@ -1,66 +0,0 @@
|
||||
# Skills Tab - Web UI Design
|
||||
|
||||
## Goal
|
||||
|
||||
Add a Skills tab to the IronClaw web gateway that lets users browse installed skills, search ClawHub for new skills, and install/remove skills -- all from the browser.
|
||||
|
||||
## Scope
|
||||
|
||||
**Frontend only.** The REST API endpoints already exist:
|
||||
|
||||
| Method | Endpoint | Purpose |
|
||||
|--------|----------|---------|
|
||||
| GET | `/api/skills` | List installed skills |
|
||||
| POST | `/api/skills/search` | Search ClawHub + local |
|
||||
| POST | `/api/skills/install` | Install (requires `X-Confirm-Action: true`) |
|
||||
| DELETE | `/api/skills/{name}` | Remove (requires `X-Confirm-Action: true`) |
|
||||
|
||||
No Rust changes needed.
|
||||
|
||||
## Layout
|
||||
|
||||
Three sections inside the tab panel:
|
||||
|
||||
### 1. Search ClawHub
|
||||
|
||||
A search input at the top. On submit, calls `POST /api/skills/search` and renders catalog results as dashed-border cards (matching the "available extension" pattern). Cards that match an already-installed skill show "Installed" instead of an Install button.
|
||||
|
||||
Staggered fade-in animation on search results for polish.
|
||||
|
||||
### 2. Installed Skills
|
||||
|
||||
Grid of cards for all locally loaded skills. Each card shows:
|
||||
- **Name** (bold, `.ext-name` style)
|
||||
- **Trust badge**: "Trusted" (green) or "Installed" (blue) -- small pill
|
||||
- **Version** (small, secondary text)
|
||||
- **Description** (`.ext-desc` style)
|
||||
- **Activation keywords** as small tags (`.ext-keywords` style)
|
||||
- **Remove button** -- only for registry-installed skills (trust=Installed), not user-placed trusted skills
|
||||
|
||||
### 3. Install by URL
|
||||
|
||||
A small form matching the WASM install form pattern:
|
||||
- Name input
|
||||
- URL input (HTTPS)
|
||||
- Install button
|
||||
|
||||
## Visual Design
|
||||
|
||||
Reuses existing `.ext-card`, `.extensions-list`, `.extensions-section`, `.btn-ext` classes. New CSS limited to:
|
||||
- `.skill-trust` badge pill (green for Trusted, blue for Installed)
|
||||
- `.skill-version` small version label
|
||||
- Staggered `@keyframes skillFadeIn` for search results
|
||||
- `.skill-search-box` for the search input styling
|
||||
|
||||
## Files Modified
|
||||
|
||||
- `src/channels/web/static/index.html` -- Add Skills tab button + panel markup
|
||||
- `src/channels/web/static/app.js` -- Add `loadSkills()`, `searchClawHub()`, `installSkill()`, `removeSkill()`, render functions, wire into `switchTab()`
|
||||
- `src/channels/web/static/style.css` -- Trust badge styles, search box, fade-in animation
|
||||
|
||||
## Decisions
|
||||
|
||||
- Reuse ext-card classes rather than creating a parallel card system
|
||||
- Trust badge differentiates skills from extensions visually
|
||||
- Confirmation uses `window.confirm()` dialog matching `removeExtension()` pattern
|
||||
- Search is manual (button/enter) not live-as-you-type to avoid hammering ClawHub
|
||||
@@ -1,574 +0,0 @@
|
||||
# Skills Tab Implementation Plan
|
||||
|
||||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.
|
||||
|
||||
**Goal:** Add a Skills tab to the IronClaw web UI for browsing installed skills, searching ClawHub, and installing/removing skills.
|
||||
|
||||
**Architecture:** Frontend-only changes to three static files (HTML, CSS, JS). The REST API (`/api/skills/*`) already exists and needs no modification. Follows the existing Extensions tab pattern: card grid layout, `apiFetch()` helper, `showToast()` for feedback.
|
||||
|
||||
**Tech Stack:** Vanilla HTML/CSS/JS (no frameworks), existing design system (CSS variables, `ext-card` family classes).
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Add Skills tab button and panel markup to index.html
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/channels/web/static/index.html:39-44` (tab bar) and `188-232` (before extensions panel)
|
||||
|
||||
**Step 1: Add the Skills tab button**
|
||||
|
||||
In `index.html`, inside the `.tab-bar` div, add a Skills button between Extensions and the spacer. Change lines 43-44 from:
|
||||
|
||||
```html
|
||||
<button data-tab="extensions">Extensions</button>
|
||||
<div class="spacer"></div>
|
||||
```
|
||||
|
||||
to:
|
||||
|
||||
```html
|
||||
<button data-tab="extensions">Extensions</button>
|
||||
<button data-tab="skills">Skills</button>
|
||||
<div class="spacer"></div>
|
||||
```
|
||||
|
||||
**Step 2: Add the Skills tab panel**
|
||||
|
||||
Add the Skills panel markup after the Extensions panel closing `</div>` (after line 232) and before the toasts div:
|
||||
|
||||
```html
|
||||
<!-- Skills Tab -->
|
||||
<div class="tab-panel" id="tab-skills">
|
||||
<div class="extensions-container">
|
||||
<div class="extensions-section">
|
||||
<h3>Search ClawHub</h3>
|
||||
<div class="skill-search-box">
|
||||
<input type="text" id="skill-search-input" placeholder="Search for skills...">
|
||||
<button onclick="searchClawHub()">Search</button>
|
||||
</div>
|
||||
<div class="extensions-list" id="skill-search-results"></div>
|
||||
</div>
|
||||
<div class="extensions-section">
|
||||
<h3>Installed Skills</h3>
|
||||
<div class="extensions-list" id="skills-list">
|
||||
<div class="empty-state">Loading skills...</div>
|
||||
</div>
|
||||
</div>
|
||||
<div class="extensions-section">
|
||||
<h3>Install Skill by URL</h3>
|
||||
<div class="ext-install-form">
|
||||
<input type="text" id="skill-install-name" placeholder="Skill name or slug">
|
||||
<input type="text" id="skill-install-url" placeholder="HTTPS URL to SKILL.md (optional)">
|
||||
<button onclick="installSkillFromForm()">Install</button>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
```
|
||||
|
||||
**Step 3: Verify the HTML is well-formed**
|
||||
|
||||
Open the file and confirm the new panel is between the Extensions panel closing tag and `<div id="toasts">`.
|
||||
|
||||
**Step 4: Commit**
|
||||
|
||||
```bash
|
||||
git add src/channels/web/static/index.html
|
||||
git commit -m "feat(web): add Skills tab markup to index.html"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Add Skills CSS (trust badges, search box, fade-in animation)
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/channels/web/static/style.css` (append before the `@media` responsive block at line 2810)
|
||||
|
||||
**Step 1: Add skill-specific CSS**
|
||||
|
||||
Insert the following CSS before the `/* --- Activity toolbar --- */` comment (before line 2810):
|
||||
|
||||
```css
|
||||
/* --- Skills tab --- */
|
||||
|
||||
.skill-search-box {
|
||||
display: flex;
|
||||
gap: 8px;
|
||||
align-items: center;
|
||||
margin-bottom: 12px;
|
||||
}
|
||||
|
||||
.skill-search-box input {
|
||||
flex: 1;
|
||||
padding: 8px 12px;
|
||||
background: var(--bg);
|
||||
border: 1px solid var(--border);
|
||||
border-radius: var(--radius);
|
||||
color: var(--text);
|
||||
font-size: 13px;
|
||||
}
|
||||
|
||||
.skill-search-box input:focus {
|
||||
outline: none;
|
||||
border-color: var(--accent);
|
||||
box-shadow: 0 0 0 3px rgba(52, 211, 153, 0.1);
|
||||
}
|
||||
|
||||
.skill-search-box button {
|
||||
padding: 8px 20px;
|
||||
background: var(--accent);
|
||||
color: #09090b;
|
||||
border: none;
|
||||
border-radius: var(--radius);
|
||||
cursor: pointer;
|
||||
font-size: 13px;
|
||||
font-weight: 600;
|
||||
transition: background 0.2s, transform 0.2s;
|
||||
}
|
||||
|
||||
.skill-search-box button:hover {
|
||||
background: var(--accent-hover);
|
||||
transform: translateY(-1px);
|
||||
}
|
||||
|
||||
.skill-trust {
|
||||
font-size: 10px;
|
||||
padding: 2px 6px;
|
||||
border-radius: 8px;
|
||||
font-weight: 500;
|
||||
text-transform: uppercase;
|
||||
letter-spacing: 0.3px;
|
||||
}
|
||||
|
||||
.skill-trust.trust-trusted {
|
||||
background: rgba(52, 211, 153, 0.15);
|
||||
color: var(--success);
|
||||
}
|
||||
|
||||
.skill-trust.trust-installed {
|
||||
background: rgba(96, 165, 250, 0.15);
|
||||
color: #60a5fa;
|
||||
}
|
||||
|
||||
.skill-version {
|
||||
font-size: 11px;
|
||||
color: var(--text-secondary);
|
||||
font-family: var(--font-mono);
|
||||
}
|
||||
|
||||
@keyframes skillFadeIn {
|
||||
from { opacity: 0; transform: translateY(8px); }
|
||||
to { opacity: 1; transform: translateY(0); }
|
||||
}
|
||||
|
||||
.skill-search-result {
|
||||
animation: skillFadeIn 0.3s ease-out both;
|
||||
}
|
||||
```
|
||||
|
||||
**Step 2: Commit**
|
||||
|
||||
```bash
|
||||
git add src/channels/web/static/style.css
|
||||
git commit -m "feat(web): add Skills tab CSS styles"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Wire Skills tab into switchTab() and keyboard shortcuts
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/channels/web/static/app.js:823-827` (switchTab function) and `2700-2704` (keyboard shortcuts)
|
||||
|
||||
**Step 1: Add skills tab loading to switchTab()**
|
||||
|
||||
In the `switchTab()` function, after line 827 (`if (tab === 'extensions') loadExtensions();`), add:
|
||||
|
||||
```javascript
|
||||
if (tab === 'skills') loadSkills();
|
||||
```
|
||||
|
||||
**Step 2: Update keyboard shortcut tab array**
|
||||
|
||||
At line 2702, change:
|
||||
|
||||
```javascript
|
||||
const tabs = ['chat', 'memory', 'jobs', 'routines', 'extensions'];
|
||||
```
|
||||
|
||||
to:
|
||||
|
||||
```javascript
|
||||
const tabs = ['chat', 'memory', 'jobs', 'routines', 'extensions', 'skills'];
|
||||
```
|
||||
|
||||
And update the key range check at line 2700 from `'5'` to `'6'`:
|
||||
|
||||
```javascript
|
||||
if (mod && e.key >= '1' && e.key <= '6') {
|
||||
```
|
||||
|
||||
**Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add src/channels/web/static/app.js
|
||||
git commit -m "feat(web): wire Skills tab into switchTab and keyboard shortcuts"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Implement loadSkills() -- render installed skills
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/channels/web/static/app.js` (add new section after the Extensions section, before keyboard shortcuts)
|
||||
|
||||
**Step 1: Add the loadSkills function**
|
||||
|
||||
Add this code block before the `// --- Keyboard shortcuts ---` comment (before line 2692):
|
||||
|
||||
```javascript
|
||||
// --- Skills ---
|
||||
|
||||
function loadSkills() {
|
||||
var skillsList = document.getElementById('skills-list');
|
||||
apiFetch('/api/skills').then(function(data) {
|
||||
if (!data.skills || data.skills.length === 0) {
|
||||
skillsList.innerHTML = '<div class="empty-state">No skills installed</div>';
|
||||
return;
|
||||
}
|
||||
skillsList.innerHTML = '';
|
||||
for (var i = 0; i < data.skills.length; i++) {
|
||||
skillsList.appendChild(renderSkillCard(data.skills[i]));
|
||||
}
|
||||
}).catch(function(err) {
|
||||
skillsList.innerHTML = '<div class="empty-state">Failed to load skills: ' + escapeHtml(err.message) + '</div>';
|
||||
});
|
||||
}
|
||||
|
||||
function renderSkillCard(skill) {
|
||||
var card = document.createElement('div');
|
||||
card.className = 'ext-card';
|
||||
|
||||
var header = document.createElement('div');
|
||||
header.className = 'ext-header';
|
||||
|
||||
var name = document.createElement('span');
|
||||
name.className = 'ext-name';
|
||||
name.textContent = skill.name;
|
||||
header.appendChild(name);
|
||||
|
||||
var trust = document.createElement('span');
|
||||
var trustClass = skill.trust.toLowerCase() === 'trusted' ? 'trust-trusted' : 'trust-installed';
|
||||
trust.className = 'skill-trust ' + trustClass;
|
||||
trust.textContent = skill.trust;
|
||||
header.appendChild(trust);
|
||||
|
||||
var version = document.createElement('span');
|
||||
version.className = 'skill-version';
|
||||
version.textContent = 'v' + skill.version;
|
||||
header.appendChild(version);
|
||||
|
||||
card.appendChild(header);
|
||||
|
||||
var desc = document.createElement('div');
|
||||
desc.className = 'ext-desc';
|
||||
desc.textContent = skill.description;
|
||||
card.appendChild(desc);
|
||||
|
||||
if (skill.keywords && skill.keywords.length > 0) {
|
||||
var kw = document.createElement('div');
|
||||
kw.className = 'ext-keywords';
|
||||
kw.textContent = 'Activates on: ' + skill.keywords.join(', ');
|
||||
card.appendChild(kw);
|
||||
}
|
||||
|
||||
var actions = document.createElement('div');
|
||||
actions.className = 'ext-actions';
|
||||
|
||||
// Only show Remove for registry-installed skills, not user-placed trusted skills
|
||||
if (skill.trust.toLowerCase() !== 'trusted') {
|
||||
var removeBtn = document.createElement('button');
|
||||
removeBtn.className = 'btn-ext remove';
|
||||
removeBtn.textContent = 'Remove';
|
||||
removeBtn.addEventListener('click', function() { removeSkill(skill.name); });
|
||||
actions.appendChild(removeBtn);
|
||||
}
|
||||
|
||||
card.appendChild(actions);
|
||||
return card;
|
||||
}
|
||||
```
|
||||
|
||||
**Step 2: Commit**
|
||||
|
||||
```bash
|
||||
git add src/channels/web/static/app.js
|
||||
git commit -m "feat(web): implement loadSkills and renderSkillCard"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 5: Implement searchClawHub() -- search and render catalog results
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/channels/web/static/app.js` (add after `renderSkillCard`, before keyboard shortcuts)
|
||||
|
||||
**Step 1: Add search and catalog card rendering**
|
||||
|
||||
Add this code after the `renderSkillCard` function:
|
||||
|
||||
```javascript
|
||||
function searchClawHub() {
|
||||
var input = document.getElementById('skill-search-input');
|
||||
var query = input.value.trim();
|
||||
if (!query) return;
|
||||
|
||||
var resultsDiv = document.getElementById('skill-search-results');
|
||||
resultsDiv.innerHTML = '<div class="empty-state">Searching...</div>';
|
||||
|
||||
apiFetch('/api/skills/search', {
|
||||
method: 'POST',
|
||||
body: { query: query },
|
||||
}).then(function(data) {
|
||||
resultsDiv.innerHTML = '';
|
||||
|
||||
// Show catalog results
|
||||
if (data.catalog && data.catalog.length > 0) {
|
||||
// Build a set of installed skill names for quick lookup
|
||||
var installedNames = {};
|
||||
if (data.installed) {
|
||||
for (var j = 0; j < data.installed.length; j++) {
|
||||
installedNames[data.installed[j].name] = true;
|
||||
}
|
||||
}
|
||||
|
||||
for (var i = 0; i < data.catalog.length; i++) {
|
||||
var card = renderCatalogSkillCard(data.catalog[i], installedNames);
|
||||
card.style.animationDelay = (i * 0.06) + 's';
|
||||
resultsDiv.appendChild(card);
|
||||
}
|
||||
}
|
||||
|
||||
// Show matching installed skills too
|
||||
if (data.installed && data.installed.length > 0) {
|
||||
for (var k = 0; k < data.installed.length; k++) {
|
||||
var installedCard = renderSkillCard(data.installed[k]);
|
||||
installedCard.style.animationDelay = ((data.catalog ? data.catalog.length : 0) + k) * 0.06 + 's';
|
||||
installedCard.classList.add('skill-search-result');
|
||||
resultsDiv.appendChild(installedCard);
|
||||
}
|
||||
}
|
||||
|
||||
if (resultsDiv.children.length === 0) {
|
||||
resultsDiv.innerHTML = '<div class="empty-state">No skills found for "' + escapeHtml(query) + '"</div>';
|
||||
}
|
||||
}).catch(function(err) {
|
||||
resultsDiv.innerHTML = '<div class="empty-state">Search failed: ' + escapeHtml(err.message) + '</div>';
|
||||
});
|
||||
}
|
||||
|
||||
function renderCatalogSkillCard(entry, installedNames) {
|
||||
var card = document.createElement('div');
|
||||
card.className = 'ext-card ext-available skill-search-result';
|
||||
|
||||
var header = document.createElement('div');
|
||||
header.className = 'ext-header';
|
||||
|
||||
var name = document.createElement('span');
|
||||
name.className = 'ext-name';
|
||||
name.textContent = entry.name || entry.slug;
|
||||
header.appendChild(name);
|
||||
|
||||
if (entry.version) {
|
||||
var version = document.createElement('span');
|
||||
version.className = 'skill-version';
|
||||
version.textContent = 'v' + entry.version;
|
||||
header.appendChild(version);
|
||||
}
|
||||
|
||||
card.appendChild(header);
|
||||
|
||||
if (entry.description) {
|
||||
var desc = document.createElement('div');
|
||||
desc.className = 'ext-desc';
|
||||
desc.textContent = entry.description;
|
||||
card.appendChild(desc);
|
||||
}
|
||||
|
||||
var actions = document.createElement('div');
|
||||
actions.className = 'ext-actions';
|
||||
|
||||
var slug = entry.slug || entry.name;
|
||||
var isInstalled = installedNames[entry.name] || installedNames[slug];
|
||||
|
||||
if (isInstalled) {
|
||||
var label = document.createElement('span');
|
||||
label.className = 'ext-active-label';
|
||||
label.textContent = 'Installed';
|
||||
actions.appendChild(label);
|
||||
} else {
|
||||
var installBtn = document.createElement('button');
|
||||
installBtn.className = 'btn-ext install';
|
||||
installBtn.textContent = 'Install';
|
||||
installBtn.addEventListener('click', (function(s, btn) {
|
||||
return function() {
|
||||
if (!confirm('Install skill "' + s + '" from ClawHub?')) return;
|
||||
btn.disabled = true;
|
||||
btn.textContent = 'Installing...';
|
||||
installSkill(s, null, btn);
|
||||
};
|
||||
})(slug, installBtn));
|
||||
actions.appendChild(installBtn);
|
||||
}
|
||||
|
||||
card.appendChild(actions);
|
||||
return card;
|
||||
}
|
||||
|
||||
// Wire up Enter key on search input
|
||||
document.getElementById('skill-search-input').addEventListener('keydown', function(e) {
|
||||
if (e.key === 'Enter') searchClawHub();
|
||||
});
|
||||
```
|
||||
|
||||
**Step 2: Commit**
|
||||
|
||||
```bash
|
||||
git add src/channels/web/static/app.js
|
||||
git commit -m "feat(web): implement ClawHub search with staggered card animation"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 6: Implement installSkill() and removeSkill()
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/channels/web/static/app.js` (add after search functions, before keyboard shortcuts)
|
||||
|
||||
**Step 1: Add install and remove functions**
|
||||
|
||||
Add this code after the search event listener:
|
||||
|
||||
```javascript
|
||||
function installSkill(nameOrSlug, url, btn) {
|
||||
var body = { name: nameOrSlug };
|
||||
if (url) body.url = url;
|
||||
|
||||
apiFetch('/api/skills/install', {
|
||||
method: 'POST',
|
||||
headers: { 'X-Confirm-Action': 'true' },
|
||||
body: body,
|
||||
}).then(function(res) {
|
||||
if (res.success) {
|
||||
showToast('Installed skill "' + nameOrSlug + '"', 'success');
|
||||
} else {
|
||||
showToast('Install failed: ' + (res.message || 'unknown error'), 'error');
|
||||
}
|
||||
loadSkills();
|
||||
if (btn) { btn.disabled = false; btn.textContent = 'Install'; }
|
||||
}).catch(function(err) {
|
||||
showToast('Install failed: ' + err.message, 'error');
|
||||
if (btn) { btn.disabled = false; btn.textContent = 'Install'; }
|
||||
});
|
||||
}
|
||||
|
||||
function removeSkill(name) {
|
||||
if (!confirm('Remove skill "' + name + '"?')) return;
|
||||
apiFetch('/api/skills/' + encodeURIComponent(name), {
|
||||
method: 'DELETE',
|
||||
headers: { 'X-Confirm-Action': 'true' },
|
||||
}).then(function(res) {
|
||||
if (res.success) {
|
||||
showToast('Removed skill "' + name + '"', 'success');
|
||||
} else {
|
||||
showToast('Remove failed: ' + (res.message || 'unknown error'), 'error');
|
||||
}
|
||||
loadSkills();
|
||||
}).catch(function(err) {
|
||||
showToast('Remove failed: ' + err.message, 'error');
|
||||
});
|
||||
}
|
||||
|
||||
function installSkillFromForm() {
|
||||
var name = document.getElementById('skill-install-name').value.trim();
|
||||
if (!name) { showToast('Skill name is required', 'error'); return; }
|
||||
var url = document.getElementById('skill-install-url').value.trim() || null;
|
||||
if (url && !url.startsWith('https://')) {
|
||||
showToast('URL must use HTTPS', 'error');
|
||||
return;
|
||||
}
|
||||
if (!confirm('Install skill "' + name + '"?')) return;
|
||||
installSkill(name, url, null);
|
||||
document.getElementById('skill-install-name').value = '';
|
||||
document.getElementById('skill-install-url').value = '';
|
||||
}
|
||||
```
|
||||
|
||||
**Step 2: Commit**
|
||||
|
||||
```bash
|
||||
git add src/channels/web/static/app.js
|
||||
git commit -m "feat(web): implement installSkill, removeSkill, and form handler"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 7: Fix apiFetch to merge extra headers properly
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/channels/web/static/app.js:86-98` (apiFetch function)
|
||||
|
||||
**Context:** The current `apiFetch` function sets `opts.headers` as an object and always overwrites with `Authorization`. When we pass `headers: { 'X-Confirm-Action': 'true' }` in options, the current code does `opts.headers = opts.headers || {}` which preserves our custom headers, then adds Authorization. However, `fetch()` expects headers as a `Headers` object or plain object -- the plain object approach works fine. Verify this works by reading the function carefully.
|
||||
|
||||
**Step 1: Verify apiFetch handles extra headers**
|
||||
|
||||
Read `app.js:86-98`. The current code:
|
||||
```javascript
|
||||
function apiFetch(path, options) {
|
||||
const opts = options || {};
|
||||
opts.headers = opts.headers || {};
|
||||
opts.headers['Authorization'] = 'Bearer ' + token;
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
This correctly merges: if we pass `{ headers: { 'X-Confirm-Action': 'true' } }`, it keeps our header and adds Authorization. **No change needed.** Move on.
|
||||
|
||||
**Step 2: Commit (skip -- no changes)**
|
||||
|
||||
---
|
||||
|
||||
### Task 8: Manual testing and final commit
|
||||
|
||||
**Step 1: Verify the HTML is valid**
|
||||
|
||||
Open `src/channels/web/static/index.html` and confirm:
|
||||
- The Skills tab button appears in the tab bar
|
||||
- The `tab-skills` panel has the correct structure
|
||||
- No unclosed tags
|
||||
|
||||
**Step 2: Verify the JS doesn't have syntax errors**
|
||||
|
||||
Run a quick syntax check (if node is available):
|
||||
```bash
|
||||
node -c src/channels/web/static/app.js
|
||||
```
|
||||
|
||||
**Step 3: Test the tab appears and loads**
|
||||
|
||||
Start the app and open the web gateway. Verify:
|
||||
1. Skills tab appears in the tab bar between Extensions and the spacer
|
||||
2. Clicking it shows the three sections
|
||||
3. Installed skills load and display with trust badges and keywords
|
||||
4. ClawHub search returns results with staggered animation
|
||||
5. Install from search works (with confirm dialog)
|
||||
6. Remove works for registry-installed skills
|
||||
7. Install by URL form works
|
||||
8. Cmd+6 keyboard shortcut switches to Skills tab
|
||||
|
||||
**Step 4: Final commit if any fixes were needed**
|
||||
|
||||
```bash
|
||||
git add src/channels/web/static/index.html src/channels/web/static/app.js src/channels/web/static/style.css
|
||||
git commit -m "feat(web): complete Skills tab with ClawHub search, install, and remove"
|
||||
```
|
||||
@@ -1,480 +0,0 @@
|
||||
# Fix Routine Silent Failures (#697) Implementation Plan
|
||||
|
||||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.
|
||||
|
||||
**Goal:** When full_job routines fail due to missing sandbox/Docker infrastructure, surface loud, clear errors to the user instead of failing silently.
|
||||
|
||||
**Architecture:** Three layers of improvement: (1) incorporate PR #711's sync mechanism so dispatched job completions/failures propagate back to routine runs, (2) fail fast at dispatch time when sandbox is configured but Docker is unavailable by threading sandbox availability into RoutineEngine, (3) send a user-visible notification at startup when sandbox is disabled due to missing Docker.
|
||||
|
||||
**Tech Stack:** Rust, tokio, thiserror
|
||||
|
||||
---
|
||||
|
||||
## Prerequisites
|
||||
|
||||
- Branch from `main` (not from the existing `fix/697-routine-silent-failure` branch)
|
||||
- We will incorporate PR #711's changes as part of this PR, making #711 superseded
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Add `list_dispatched_routine_runs` to Database trait and implementations
|
||||
|
||||
PR #711 adds this method. We incorporate it here.
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/db/mod.rs` (RoutineStore trait)
|
||||
- Modify: `src/db/postgres.rs`
|
||||
- Modify: `src/db/libsql/routines.rs`
|
||||
- Modify: `src/history/store.rs`
|
||||
|
||||
**Step 1: Add trait method to RoutineStore**
|
||||
|
||||
In `src/db/mod.rs`, add to the `RoutineStore` trait (after `link_routine_run_to_job`):
|
||||
|
||||
```rust
|
||||
/// List routine runs that were dispatched as full_job (status = 'running'
|
||||
/// with a linked job_id). Used by the routine engine to sync completion
|
||||
/// status from the background job.
|
||||
async fn list_dispatched_routine_runs(&self) -> Result<Vec<RoutineRun>, DatabaseError>;
|
||||
```
|
||||
|
||||
**Step 2: Implement for PostgreSQL**
|
||||
|
||||
In `src/db/postgres.rs`, add the implementation (delegating to `Store`):
|
||||
|
||||
```rust
|
||||
async fn list_dispatched_routine_runs(&self) -> Result<Vec<RoutineRun>, DatabaseError> {
|
||||
self.inner.list_dispatched_routine_runs().await
|
||||
}
|
||||
```
|
||||
|
||||
**Step 3: Implement for libSQL**
|
||||
|
||||
In `src/db/libsql/routines.rs`, add:
|
||||
|
||||
```rust
|
||||
pub async fn list_dispatched_routine_runs(
|
||||
&self,
|
||||
) -> Result<Vec<RoutineRun>, DatabaseError> {
|
||||
let conn = self.pool.connection().await.map_err(|e| {
|
||||
DatabaseError::Query(format!("failed to get connection: {e}"))
|
||||
})?;
|
||||
let mut rows = conn
|
||||
.query(
|
||||
"SELECT id, routine_id, trigger_type, trigger_detail, started_at, \
|
||||
completed_at, status, result_summary, tokens_used, job_id, created_at \
|
||||
FROM routine_runs WHERE status = 'running' AND job_id IS NOT NULL",
|
||||
(),
|
||||
)
|
||||
.await
|
||||
.map_err(|e| DatabaseError::Query(e.to_string()))?;
|
||||
|
||||
let mut runs = Vec::new();
|
||||
while let Some(row) = rows.next().await.map_err(|e| DatabaseError::Query(e.to_string()))? {
|
||||
runs.push(parse_routine_run_row(&row)?);
|
||||
}
|
||||
Ok(runs)
|
||||
}
|
||||
```
|
||||
|
||||
**Step 4: Implement for Store wrapper**
|
||||
|
||||
In `src/history/store.rs`, add:
|
||||
|
||||
```rust
|
||||
pub async fn list_dispatched_routine_runs(&self) -> Result<Vec<RoutineRun>, DatabaseError> {
|
||||
sqlx::query_as::<_, RoutineRunRow>(
|
||||
"SELECT id, routine_id, trigger_type, trigger_detail, started_at, \
|
||||
completed_at, status, result_summary, tokens_used, job_id, created_at \
|
||||
FROM routine_runs WHERE status = 'running' AND job_id IS NOT NULL"
|
||||
)
|
||||
.fetch_all(&self.pool)
|
||||
.await
|
||||
.map(|rows| rows.into_iter().map(Into::into).collect())
|
||||
.map_err(|e| DatabaseError::Query(e.to_string()))
|
||||
}
|
||||
```
|
||||
|
||||
**Step 5: Verify compilation**
|
||||
|
||||
```bash
|
||||
cargo check
|
||||
cargo check --no-default-features --features libsql
|
||||
```
|
||||
|
||||
**Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add src/db/mod.rs src/db/postgres.rs src/db/libsql/routines.rs src/history/store.rs
|
||||
git commit -m "feat(db): add list_dispatched_routine_runs for routine-job sync (#697)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Add sync_dispatched_runs and fix dispatch status in routine_engine
|
||||
|
||||
Incorporates PR #711's core fix: change `execute_full_job` to return `RunStatus::Running` instead of `Ok`, and add the periodic sync mechanism.
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/agent/routine_engine.rs`
|
||||
|
||||
**Step 1: Write tests for job-state-to-run-status mapping and Running notification gating**
|
||||
|
||||
Add to the `mod tests` block at the bottom of `routine_engine.rs`:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn test_running_status_does_not_notify() {
|
||||
let config = NotifyConfig {
|
||||
on_success: true,
|
||||
on_failure: true,
|
||||
on_attention: true,
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let should_notify = match RunStatus::Running {
|
||||
RunStatus::Ok => config.on_success,
|
||||
RunStatus::Attention => config.on_attention,
|
||||
RunStatus::Failed => config.on_failure,
|
||||
RunStatus::Running => false,
|
||||
};
|
||||
assert!(!should_notify);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_full_job_dispatch_returns_running_status() {
|
||||
assert_eq!(RunStatus::Running.to_string(), "running");
|
||||
}
|
||||
|
||||
/// Regression test for #697: full_job routines were immediately marked Ok
|
||||
/// on dispatch, so failures/completions were never synced back.
|
||||
#[test]
|
||||
fn test_job_state_to_run_status_mapping() {
|
||||
use crate::context::JobState;
|
||||
|
||||
let map_state = |state: JobState, reason: Option<&str>| -> Option<(RunStatus, String)> {
|
||||
let last_reason = reason.map(|s| s.to_string());
|
||||
match state {
|
||||
JobState::Completed | JobState::Submitted | JobState::Accepted => {
|
||||
let summary =
|
||||
last_reason.unwrap_or_else(|| "Job completed successfully".to_string());
|
||||
Some((RunStatus::Ok, summary))
|
||||
}
|
||||
JobState::Failed => {
|
||||
let summary = last_reason
|
||||
.unwrap_or_else(|| "Job failed (no error message recorded)".to_string());
|
||||
Some((RunStatus::Failed, summary))
|
||||
}
|
||||
JobState::Cancelled => Some((RunStatus::Failed, "Job was cancelled".to_string())),
|
||||
JobState::Pending | JobState::InProgress | JobState::Stuck => None,
|
||||
}
|
||||
};
|
||||
|
||||
let (status, _) = map_state(JobState::Completed, None).unwrap();
|
||||
assert_eq!(status, RunStatus::Ok);
|
||||
|
||||
let (status, _) = map_state(JobState::Failed, Some("OOM killed")).unwrap();
|
||||
assert_eq!(status, RunStatus::Failed);
|
||||
assert_eq!(summary, "OOM killed");
|
||||
|
||||
let (status, summary) = map_state(JobState::Failed, None).unwrap();
|
||||
assert_eq!(status, RunStatus::Failed);
|
||||
assert!(summary.contains("no error message"));
|
||||
|
||||
assert!(map_state(JobState::Pending, None).is_none());
|
||||
assert!(map_state(JobState::InProgress, None).is_none());
|
||||
assert!(map_state(JobState::Stuck, None).is_none());
|
||||
}
|
||||
```
|
||||
|
||||
**Step 2: Run tests to verify they fail**
|
||||
|
||||
```bash
|
||||
cargo test routine_engine::tests --all-features
|
||||
```
|
||||
|
||||
Expected: compilation error since `sync_dispatched_runs` doesn't exist yet.
|
||||
|
||||
**Step 3: Add import and sync methods**
|
||||
|
||||
Add `use crate::context::JobState;` to the imports.
|
||||
|
||||
Add `sync_dispatched_runs` and `complete_dispatched_run` methods to `impl RoutineEngine` (after `check_cron_triggers`). See PR #711 diff for exact implementation.
|
||||
|
||||
Change `execute_full_job` return from:
|
||||
```rust
|
||||
Ok((RunStatus::Ok, Some(summary), None))
|
||||
```
|
||||
to:
|
||||
```rust
|
||||
Ok((RunStatus::Running, Some(summary), None))
|
||||
```
|
||||
|
||||
Update the summary message to include "Status will be updated when the job completes."
|
||||
|
||||
Add `engine.sync_dispatched_runs().await;` to the cron ticker loop in `spawn_cron_ticker`, after `check_cron_triggers`.
|
||||
|
||||
**Step 4: Run tests**
|
||||
|
||||
```bash
|
||||
cargo test routine_engine::tests --all-features
|
||||
```
|
||||
|
||||
Expected: PASS
|
||||
|
||||
**Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add src/agent/routine_engine.rs
|
||||
git commit -m "fix(routines): sync dispatched full_job runs with job completion (#697)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Fail fast when sandbox is unavailable at dispatch time
|
||||
|
||||
This is the new work beyond PR #711. Thread sandbox availability into `RoutineEngine` so `execute_full_job` can fail immediately with a clear error instead of dispatching a doomed job.
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/agent/routine_engine.rs`
|
||||
- Modify: `src/agent/agent_loop.rs`
|
||||
|
||||
**Step 1: Write the failing test**
|
||||
|
||||
Add to `mod tests` in `routine_engine.rs`:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn test_sandbox_unavailable_error_message() {
|
||||
let err = RoutineError::JobDispatchFailed {
|
||||
reason: "Sandbox is enabled but Docker is not available. \
|
||||
Install Docker or set SANDBOX_ENABLED=false to run full_job routines."
|
||||
.to_string(),
|
||||
};
|
||||
let msg = err.to_string();
|
||||
assert!(msg.contains("Docker is not available"));
|
||||
assert!(msg.contains("SANDBOX_ENABLED"));
|
||||
}
|
||||
```
|
||||
|
||||
**Step 2: Run test to verify it passes (this one is a unit test for the error variant)**
|
||||
|
||||
```bash
|
||||
cargo test routine_engine::tests::test_sandbox_unavailable_error_message --all-features
|
||||
```
|
||||
|
||||
Expected: PASS (error variant already exists, we're just testing the message).
|
||||
|
||||
**Step 3: Add `sandbox_available` field to `RoutineEngine`**
|
||||
|
||||
In `src/agent/routine_engine.rs`, add a field to the `RoutineEngine` struct:
|
||||
|
||||
```rust
|
||||
/// Whether sandbox/Docker infrastructure is available for full_job execution.
|
||||
sandbox_available: bool,
|
||||
```
|
||||
|
||||
Update `RoutineEngine::new` to accept and store it:
|
||||
|
||||
```rust
|
||||
pub fn new(
|
||||
config: RoutineConfig,
|
||||
store: Arc<dyn Database>,
|
||||
llm: Arc<dyn LlmProvider>,
|
||||
workspace: Arc<Workspace>,
|
||||
notify_tx: mpsc::Sender<OutgoingResponse>,
|
||||
scheduler: Option<Arc<Scheduler>>,
|
||||
sandbox_available: bool,
|
||||
) -> Self {
|
||||
Self {
|
||||
config,
|
||||
store,
|
||||
llm,
|
||||
workspace,
|
||||
notify_tx,
|
||||
running_count: Arc::new(AtomicUsize::new(0)),
|
||||
event_cache: Arc::new(RwLock::new(Vec::new())),
|
||||
scheduler,
|
||||
sandbox_available,
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Step 4: Add sandbox check in `execute_full_job`**
|
||||
|
||||
At the top of `execute_full_job`, before the scheduler check, add a sandbox availability check. This requires passing `sandbox_available` through `EngineContext`.
|
||||
|
||||
Add `sandbox_available: bool` to `EngineContext`.
|
||||
|
||||
Update `spawn_fire` and `fire_manual` to pass `self.sandbox_available` into `EngineContext`.
|
||||
|
||||
In `execute_full_job`, add before the scheduler check:
|
||||
|
||||
```rust
|
||||
if !ctx.sandbox_available {
|
||||
return Err(RoutineError::JobDispatchFailed {
|
||||
reason: "Sandbox is enabled but Docker is not available. \
|
||||
Install Docker or set SANDBOX_ENABLED=false to run full_job routines."
|
||||
.to_string(),
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
**Step 5: Update call site in `agent_loop.rs`**
|
||||
|
||||
In `src/agent/agent_loop.rs`, where `RoutineEngine::new` is called (~line 442), pass the sandbox availability. The `Agent` struct needs to know Docker status. The simplest approach:
|
||||
|
||||
Add a `sandbox_available: bool` field to `Agent` (or to `AgentDeps`). Set it during construction based on the `docker_status` from `main.rs`. The value flows: `main.rs` detects Docker -> passes `sandbox_available` bool through `AppComponents` or `AgentDeps` -> `Agent` passes it to `RoutineEngine::new`.
|
||||
|
||||
Look at how `main.rs` passes config to `Agent`. The `docker_status` is computed in `main.rs`. The cleanest path:
|
||||
- Add `sandbox_available: bool` to `AppComponents` (set in `main.rs`)
|
||||
- Thread it through to `AgentDeps` -> `Agent` -> `RoutineEngine::new`
|
||||
|
||||
Alternatively, since `config.sandbox.enabled` is already available in the agent, just add one more bool. Check the existing flow and pick the minimal path.
|
||||
|
||||
**Step 6: Verify compilation**
|
||||
|
||||
```bash
|
||||
cargo check --all-features
|
||||
cargo check --no-default-features --features libsql
|
||||
```
|
||||
|
||||
**Step 7: Run tests**
|
||||
|
||||
```bash
|
||||
cargo test routine_engine::tests --all-features
|
||||
```
|
||||
|
||||
**Step 8: Commit**
|
||||
|
||||
```bash
|
||||
git add src/agent/routine_engine.rs src/agent/agent_loop.rs src/main.rs src/app.rs
|
||||
git commit -m "fix(routines): fail fast when sandbox unavailable at dispatch time (#697)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Surface sandbox unavailability to user via notification channel
|
||||
|
||||
Currently the Docker detection warning only goes to `tracing::warn` (logs). Users on TUI/web never see it. Send a user-visible notification after channels are set up.
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/main.rs`
|
||||
|
||||
**Step 1: Write the test**
|
||||
|
||||
This is a startup behavior change, so the test is an integration-level assertion. Add a unit test for the notification message formatting:
|
||||
|
||||
In `src/agent/routine_engine.rs` tests (or a new test in main.rs tests if they exist):
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn test_sandbox_warning_message_format() {
|
||||
let msg = format!(
|
||||
"Sandbox is enabled but Docker is not available -- full_job routines will fail. {}",
|
||||
"Install Docker Desktop from https://docker.com/get-started"
|
||||
);
|
||||
assert!(msg.contains("full_job routines will fail"));
|
||||
assert!(msg.contains("Docker"));
|
||||
}
|
||||
```
|
||||
|
||||
**Step 2: Add startup notification in `main.rs`**
|
||||
|
||||
After the channel manager is set up and the agent is running, if `config.sandbox.enabled && !docker_status.is_ok()`, send a warning message through the channel manager. The pattern already exists for heartbeat/routine notifications.
|
||||
|
||||
The exact location: after `channels` is fully initialized (after all channels are added), but before the agent run loop. Find where `channels.broadcast_all` is accessible.
|
||||
|
||||
The simplest approach: after the agent starts (`agent.run()` is typically the last call), but since that blocks, the notification should be sent *before* `agent.run()` is called, using a spawned task or inline send.
|
||||
|
||||
Look at where heartbeat startup notifications go. Mirror that pattern:
|
||||
|
||||
```rust
|
||||
if config.sandbox.enabled && !docker_status.is_ok() {
|
||||
let warning = format!(
|
||||
"Warning: Sandbox is enabled but Docker is not available -- \
|
||||
full_job routines will fail until Docker is running. {}",
|
||||
docker_status_detection.platform.install_hint()
|
||||
);
|
||||
let response = OutgoingResponse {
|
||||
content: warning,
|
||||
thread_id: None,
|
||||
attachments: Vec::new(),
|
||||
metadata: serde_json::json!({
|
||||
"source": "system",
|
||||
"type": "warning",
|
||||
}),
|
||||
};
|
||||
let channels_clone = channels.clone();
|
||||
tokio::spawn(async move {
|
||||
// Small delay to let channels finish connecting
|
||||
tokio::time::sleep(std::time::Duration::from_secs(2)).await;
|
||||
let _ = channels_clone.broadcast_all("default", response).await;
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
Note: we need to preserve the `detection` struct (not just `docker_status`) to access `platform.install_hint()`. Adjust the variable binding in the Docker detection block to keep it available.
|
||||
|
||||
**Step 3: Verify compilation**
|
||||
|
||||
```bash
|
||||
cargo check --all-features
|
||||
```
|
||||
|
||||
**Step 4: Commit**
|
||||
|
||||
```bash
|
||||
git add src/main.rs
|
||||
git commit -m "feat(startup): notify user when sandbox unavailable (#697)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 5: Final verification and cleanup
|
||||
|
||||
**Step 1: Run full test suite**
|
||||
|
||||
```bash
|
||||
cargo fmt
|
||||
cargo clippy --all --benches --tests --examples --all-features
|
||||
cargo test --all-features
|
||||
```
|
||||
|
||||
**Step 2: Verify both feature configurations compile**
|
||||
|
||||
```bash
|
||||
cargo check --no-default-features --features libsql
|
||||
cargo check
|
||||
```
|
||||
|
||||
**Step 3: Run pre-commit safety checks**
|
||||
|
||||
```bash
|
||||
grep -rnE '\.unwrap\(|\.expect\(' src/agent/routine_engine.rs src/main.rs
|
||||
```
|
||||
|
||||
Expect: no hits in production code (test code is fine).
|
||||
|
||||
**Step 4: Create final commit if any formatting/clippy fixes needed**
|
||||
|
||||
```bash
|
||||
git add -A
|
||||
git commit -m "style: formatting and clippy fixes (#697)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Summary of Changes
|
||||
|
||||
| What | Where | Why |
|
||||
|------|-------|-----|
|
||||
| `list_dispatched_routine_runs` DB method | `db/mod.rs`, postgres, libsql, store | Query for running routine runs with linked jobs |
|
||||
| `sync_dispatched_runs()` engine method | `routine_engine.rs` | Periodically sync job completion back to routine runs |
|
||||
| `RunStatus::Running` on dispatch | `routine_engine.rs` | Don't mark as Ok before job actually completes |
|
||||
| `sandbox_available` flag | `RoutineEngine`, `EngineContext` | Fail fast at dispatch when Docker missing |
|
||||
| Startup notification | `main.rs` | Warn user visibly when sandbox is disabled |
|
||||
|
||||
## PR Scope
|
||||
|
||||
This PR supersedes PR #711 by incorporating its changes plus the additional fail-fast and startup notification work. PR #711 can be closed after this merges.
|
||||
@@ -1,82 +0,0 @@
|
||||
# Security Merge Train Status Board
|
||||
|
||||
Date opened: 2026-03-11
|
||||
Last updated: 2026-03-12
|
||||
Base branch: `staging`
|
||||
Current `staging` head: `acea1143cf70f7fa593c077620c979d5aa260de9`
|
||||
|
||||
This board started as the security merge train plan and now tracks the live status of the approved-PR merge effort.
|
||||
|
||||
## Current Branch Health
|
||||
|
||||
- Full staging batch for `acea1143cf70f7fa593c077620c979d5aa260de9` completed green.
|
||||
- E2E, Linux tests, Windows builds, Docker build, WASM WIT compatibility, staging gate, and summary all passed.
|
||||
- Current gating problem is no longer branch regressions. It is fresh review requirements on replacement PRs.
|
||||
|
||||
## Merged Into `staging`
|
||||
|
||||
| PR | Title | Outcome |
|
||||
|---|---|---|
|
||||
| #510 | fix(security): add DOMPurify and sanitize rendered markdown | Merged |
|
||||
| #518 | fix(security): resolve DNS once and reuse for SSRF validation | Merged |
|
||||
| #520 | fix(security): harden auth token env overlay usage / WASM metadata loading hardening | Merged |
|
||||
| #949 | fix(setup): drain residual events and filter key kind in onboard prompts | Merged |
|
||||
| #935 | fix(mcp): stdio/unix transports skip initialize handshake | Merged |
|
||||
| #760 | fix(agent): block thread_id-based context pollution across users | Merged |
|
||||
| #752 | fix(mcp): header safety validation and Authorization conflict bug from #704 | Merged |
|
||||
| #735 | fix: drain tunnel pipes to prevent zombie process | Merged |
|
||||
| #684 | fix(setup): validate channel credentials during setup | Merged |
|
||||
| #850 | docs: add Russian localization (README.ru.md) | Merged |
|
||||
| #851 | feat(setup): display ASCII art banner during onboarding | Merged |
|
||||
| #964 | fix(ci): disambiguate WASM bundle filenames to prevent tool/channel collision | Merged |
|
||||
| #839 | fix(test): stabilize openai compat oversized-body regression | Merged |
|
||||
| #472 | Fix systemctl unit | Merged |
|
||||
|
||||
## Security Replacement Queue
|
||||
|
||||
These supersede the originally approved but dirty security PRs.
|
||||
|
||||
| Replacement PR | Supersedes | CI | Auto-merge | Merge blocker | Notes |
|
||||
|---|---|---|---|---|---|
|
||||
| #966 | #514 | Green | Enabled | `REVIEW_REQUIRED` | CSP replacement; includes E2E coverage |
|
||||
| #967 | #516 | Green | Enabled | `REVIEW_REQUIRED` | FullAccess policy guard |
|
||||
| #968 | #522 | Green | Enabled | `REVIEW_REQUIRED` | Safe env overlay / set_var invariants |
|
||||
| #970 | #513 | Green | Enabled | `REVIEW_REQUIRED` | Webhook HMAC migration |
|
||||
|
||||
## General Replacement Queue
|
||||
|
||||
These supersede other approved dirty PRs that were still worth carrying forward.
|
||||
|
||||
| Replacement PR | Supersedes | CI | Auto-merge | Merge blocker | Notes |
|
||||
|---|---|---|---|---|---|
|
||||
| #986 | #793 | Green | Enabled | `REVIEW_REQUIRED` | Non-OAuth HTTP MCP clients now carry session manager |
|
||||
| #987 | #679 | In progress / early checks green | Enabled | `REVIEW_REQUIRED` | Preserves `selected_model` when re-running setup on the same backend |
|
||||
|
||||
## Approved Originals Still Open
|
||||
|
||||
| PR | Title | Current state | Recommended action | Notes |
|
||||
|---|---|---|---|---|
|
||||
| #514 | fix(security): add Content-Security-Policy header to web gateway | Dirty | Ignore in favor of #966 | Replacement path is active |
|
||||
| #516 | fix(security): require explicit `SANDBOX_ALLOW_FULL_ACCESS` to enable FullAccess policy | Dirty | Ignore in favor of #967 | Replacement path is active |
|
||||
| #522 | fix(security): make unsafe `env::set_var` calls safe with explicit invariants | Dirty | Ignore in favor of #968 | Replacement path is active |
|
||||
| #513 | fix(security): migrate webhook auth to HMAC-SHA256 signature header | Dirty | Ignore in favor of #970 | Replacement path is active |
|
||||
| #793 | fix(mcp): set session manager on non-OAuth HTTP MCP clients | Dirty | Ignore in favor of #986 | Replacement path is active |
|
||||
| #679 | fix(setup): preserve model selection on provider re-run | Dirty | Ignore in favor of #987 | Replacement path is active |
|
||||
| #737 | 汉化v0.1.0 | Dirty | Do not open a faithful replacement | `staging` already has a divergent i18n implementation |
|
||||
| #831 | refactor(orchestrator/api): use `test_secrets_store()` helper in credentials test | Dirty + draft | Do not rescue as-is | Current diff has drifted far beyond the title / intended scope |
|
||||
| #934 | fix(memory): reject absolute filesystem paths with corrective routing | Unstable | Do not merge as-is | Default-branch workflow change would break staging promotion in this repo |
|
||||
| #616 | feat: adds context-llm tool support | Unstable | Separate review pass needed | Too large for the safe merge train |
|
||||
|
||||
## Practical Merge Order From Here
|
||||
|
||||
1. Get fresh approval on `#966`, `#967`, `#968`, `#970`, `#986`, `#987`.
|
||||
2. Let auto-merge land them as checks clear.
|
||||
3. Re-run full staging CI after each actual merge to `staging`.
|
||||
4. Treat `#934`, `#737`, `#831`, and `#616` as separate workstreams, not part of the current safe merge train.
|
||||
|
||||
## Key Findings
|
||||
|
||||
- The repo token used here cannot bypass the required-review ruleset, even with `gh pr merge --admin`.
|
||||
- Direct pushes to `staging` are blocked by repo rules (`GH013`).
|
||||
- The replacement-PR path is the workable route for approved dirty PRs.
|
||||
- `#934` is not merely stale. Its workflow change is unsafe here because this repository's default branch is `staging`, not `main`.
|
||||
@@ -1,184 +0,0 @@
|
||||
# Engine v2 Acceptance Criteria
|
||||
|
||||
**Date:** 2026-03-22
|
||||
**Status:** Active
|
||||
**Author:** Zaki Manian
|
||||
**Goal:** Define the merge bar for replacing the v1 agent loop with the v2 engine (`crates/ironclaw_engine/`). Phase 6 is not done until every criterion below is met.
|
||||
|
||||
---
|
||||
|
||||
## Overview
|
||||
|
||||
The v2 engine replaces ~10 v1 abstractions (Session, Job, Routine, Channel, Tool, Skill, Hook, Observer, Extension, LoopDelegate) with 5 primitives (Thread, Step, Capability, MemoryDoc, Project). Phases 1-5 are complete: types, execution loop, CodeAct/Monty, budget controls, and conversation surface.
|
||||
|
||||
Phase 6 delivers the bridge adapters (`LlmBridgeAdapter`, `StoreBridgeAdapter`, `EffectBridgeAdapter`) that connect the engine to existing IronClaw infrastructure. The acceptance criteria below define what "ready to replace v1" means. Nothing merges to `staging` until all pass.
|
||||
|
||||
---
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
### 1. Behavioral Equivalence
|
||||
|
||||
Every observable behavior of v1 must be reproduced by v2 running through bridge adapters.
|
||||
|
||||
| # | Criterion | Verification |
|
||||
|---|-----------|-------------|
|
||||
| 1.1 | All existing E2E test fixtures pass through `EngineV2Delegate` | `cargo test --features integration -p ironclaw -- engine_v2` and `cd tests/e2e && pytest` with `ENGINE_V2=true` |
|
||||
| 1.2 | Tool dispatch produces identical outputs for identical inputs | Add property test: for each built-in tool, run same `(name, params)` through v1 `execute_tool_with_safety()` and v2 `EffectBridgeAdapter::execute_action()`, assert outputs match |
|
||||
| 1.3 | Error handling is equivalent: no silent failures where v1 errors, no errors where v1 succeeds | Diff test: run full E2E trace fixtures through both paths, compare `LoopOutcome` variants. Specifically test: invalid tool name, malformed params, timeout, policy deny |
|
||||
| 1.4 | Approval flows work identically | Test sequence: tool with `requires_approval` -> pause -> user approves -> resume -> completion. Must produce same SSE events (`approval_needed`, `approval_resolved`) |
|
||||
| 1.5 | System commands (`/help`, `/model`, `/status`, `/skills`, `/job`) produce equivalent responses | Command parity test: submit each system command through v2 conversation surface, compare output structure |
|
||||
| 1.6 | Compaction produces equivalent context reduction | Run a 50-turn conversation through both engines, trigger compaction, compare resulting context window token count (must be within 5%) |
|
||||
|
||||
**Blocking:** 1.1, 1.2, 1.3, 1.4 are hard blockers. 1.5 and 1.6 may be deferred to Phase 7 with written justification.
|
||||
|
||||
### 2. Performance
|
||||
|
||||
No performance regressions. Improvements expected from context-as-variables but not required.
|
||||
|
||||
| # | Criterion | Target | Verification |
|
||||
|---|-----------|--------|-------------|
|
||||
| 2.1 | P50 step latency | Within +10% of v1 | Benchmark harness: `cargo bench -p ironclaw --bench step_latency` with mock LLM (fixed 50ms response). Run 1000 steps, compare distributions. Harness must test both engines in the same binary. |
|
||||
| 2.2 | P95 step latency | Within +10% of v1 | Same harness as 2.1 |
|
||||
| 2.3 | P99 step latency | Within +15% of v1 | Same harness as 2.1 (wider margin for tail latency) |
|
||||
| 2.4 | Monty VM startup | < 1ms (verify the 0.06ms claim) | Dedicated microbenchmark: `cargo bench -p ironclaw_engine --bench monty_startup`. Time `MontyVm::new()` over 10,000 iterations, report P50/P99. Must include independent measurement, not self-reported. |
|
||||
| 2.5 | Token efficiency | Neutral or improved | Measure total tokens (prompt + completion) for the same 10-turn conversation fixture through both engines. v2 must not use more tokens than v1. Context-as-variables should reduce prompt tokens by 10-30% on conversations with tool output > 4KB. |
|
||||
| 2.6 | Memory per thread | No regression | Measure RSS delta when spawning 100 threads with mock LLM. v2 must not exceed v1 by more than 10%. |
|
||||
|
||||
**Blocking:** 2.1, 2.2, 2.3 are hard blockers. 2.4, 2.5, 2.6 are soft blockers (documented regressions acceptable with mitigation plan).
|
||||
|
||||
### 3. Safety and Security
|
||||
|
||||
The engine itself contains no safety logic by design. Safety is enforced at the bridge boundary (`EffectBridgeAdapter`). This must be airtight.
|
||||
|
||||
| # | Criterion | Verification |
|
||||
|---|-----------|-------------|
|
||||
| 3.1 | `SafetyLayer` (prompt injection, leak detection, content validation) is applied on every action execution through `EffectBridgeAdapter` | Unit test: mock `EffectExecutor` that logs calls, verify `SafetyLayer::validate_tool_input()` and `SafetyLayer::sanitize_tool_output()` are called for every `execute_action()` invocation. No code path bypasses this. |
|
||||
| 3.2 | Policy engine enforces `Deny > RequireApproval > Allow` with zero bypasses | Test matrix: for each `EffectType` variant (ReadLocal, ReadExternal, WriteLocal, WriteExternal, CredentialedNetwork, Compute, Financial), create conflicting rules and verify Deny always wins, RequireApproval wins over Allow. Cover the case where a single action triggers multiple effect types. |
|
||||
| 3.3 | Thread tree is acyclic with bounded depth | `ThreadTree::attach()` must reject cycles (test: A->B->C->A). `ThreadConfig::max_depth` must be enforced (test: exceed depth limit, verify `ThreadError::DepthExceeded`). Default max depth: 8. |
|
||||
| 3.4 | Capability leases are checked before every action execution | Audit `ExecutionLoop::run()` and `execute_action_calls()`: no path from LLM response to `EffectExecutor::execute_action()` that skips `LeaseManager::check_lease()`. Verify with test: expired lease -> action denied, revoked lease -> action denied, exhausted `max_uses` -> action denied. |
|
||||
| 3.5 | Monty VM panics cannot crash the host | Test: inject Python code that triggers a Monty panic (e.g., stack overflow, infinite allocation). Verify the step completes with `StepStatus::Failed`, thread continues or fails gracefully, no process abort. Specifically test all resource limits: 30s timeout, 64MB memory, 1M allocations. |
|
||||
| 3.6 | No new attack surfaces | Review checklist (manual, documented in PR): (a) lease forgery: `LeaseId` cannot be guessed or constructed outside `LeaseManager::grant()`, (b) policy bypass: no public method on `ExecutionLoop` that executes actions without policy check, (c) effect escalation: action's declared `EffectType` cannot be changed after capability registration, (d) cross-thread lease usage: lease bound to `thread_id` is enforced. |
|
||||
|
||||
**Blocking:** All items are hard blockers. 3.6 is a manual review checklist that must be signed off in the merge PR.
|
||||
|
||||
### 4. Persistence and Migration
|
||||
|
||||
Production requires durable state. `InMemoryStore` is for tests only.
|
||||
|
||||
| # | Criterion | Verification |
|
||||
|---|-----------|-------------|
|
||||
| 4.1 | `StoreBridgeAdapter` implements the full `Store` trait (18 methods) for both PostgreSQL and libSQL | Integration test per backend: create thread -> add steps -> append events -> save leases -> restart process -> load thread -> verify all data intact. Run with `cargo test --features integration` (postgres) and default (libSQL). |
|
||||
| 4.2 | Database migrations create all required tables | Migration V14+ creates: `engine_threads`, `engine_steps`, `engine_events`, `engine_projects`, `engine_memory_docs`, `engine_capability_leases`. Test: run migrations on empty database, verify tables exist with correct schemas. Both backends. |
|
||||
| 4.3 | Thread state survives process restart | Integration test: start thread -> execute 3 steps -> kill process -> restart -> resume thread -> verify step count is 3, thread state is correct, events are intact. |
|
||||
| 4.4 | In-flight v1 sessions continue working when v2 is enabled | Test: create v1 session with active thread -> enable `ENGINE_V2=true` -> new messages on the existing session use v1 path (not v2). Only new threads use v2. Verify with assertion on delegate type. |
|
||||
| 4.5 | Data migration path is documented | `docs/plans/` must contain a migration guide covering: (a) which v1 tables map to which v2 tables, (b) whether historical data is migrated or v2 starts fresh, (c) rollback procedure if migration fails. |
|
||||
|
||||
**Blocking:** 4.1, 4.2, 4.3, 4.4 are hard blockers. 4.5 is required documentation but may ship as a separate document in the same milestone.
|
||||
|
||||
### 5. Observability
|
||||
|
||||
The engine must emit enough telemetry to debug production issues without attaching a debugger.
|
||||
|
||||
| # | Criterion | Verification |
|
||||
|---|-----------|-------------|
|
||||
| 5.1 | Step execution duration is recorded | Each `Step` must have `started_at` and `completed_at` timestamps. Verify via unit test: execute a step, assert both fields are set and `completed_at > started_at`. |
|
||||
| 5.2 | Token usage is tracked per step and per thread | `Step::token_usage` must be populated from `LlmOutput`. Thread-level aggregation: `thread.steps.iter().map(|s| s.token_usage).sum()`. Verify: run 5 steps with known token counts from mock LLM, assert thread total matches. |
|
||||
| 5.3 | Policy decision counters | `PolicyEngine` must expose counts of `Allow`, `Deny`, and `RequireApproval` decisions. Verify: run 10 actions with mixed policies, assert counters match expected values. These must be queryable (not just logged). |
|
||||
| 5.4 | Active lease gauge | `LeaseManager` must expose current active lease count. Verify: grant 5 leases, revoke 2, expire 1, assert gauge reads 2. |
|
||||
| 5.5 | Event sourcing query performance | `Store::load_events(thread_id)` must return within 100ms for a thread with 1000 events. Benchmark test with both backends. |
|
||||
| 5.6 | Structured logging for execution loop | Each step must emit `tracing` spans with: `thread_id`, `step_index`, `execution_tier`, `duration_ms`, `token_count`. Verify by capturing tracing output in test and asserting field presence. |
|
||||
|
||||
**Blocking:** 5.1, 5.2, 5.6 are hard blockers. 5.3, 5.4, 5.5 are soft blockers (must be filed as issues if deferred).
|
||||
|
||||
### 6. Rollout Strategy
|
||||
|
||||
No big-bang cutover. Gradual rollout with rollback capability.
|
||||
|
||||
| # | Criterion | Verification |
|
||||
|---|-----------|-------------|
|
||||
| 6.1 | Feature flag `ENGINE_V2` controls engine selection | When `ENGINE_V2=true`: new threads use `EngineV2Delegate`. When `ENGINE_V2=false` (default): all threads use v1. Verify: start with flag off, create thread (v1), set flag on, create thread (v2), both work. |
|
||||
| 6.2 | Existing threads continue on their original engine | A thread started on v1 must remain on v1 even when `ENGINE_V2=true`. Thread metadata must record which engine version created it. Verify: create v1 thread, enable v2, send message to v1 thread, assert v1 delegate is used. |
|
||||
| 6.3 | Rollback path: disable flag, no data loss | Enable v2, create threads, disable v2. v2 threads become read-only (no new messages accepted) but their data persists. New threads use v1. No data corruption in either direction. |
|
||||
| 6.4 | Percentage-based rollout support | `ENGINE_V2_ROLLOUT_PERCENT=10` routes 10% of new threads to v2 (hash of thread_id mod 100). This enables canary deployment. Verify: create 100 threads with rollout at 10%, assert approximately 10 use v2. |
|
||||
| 6.5 | Canary validation period | Before full rollout, v2 must run on >= 10% of new threads for at least 1 week with no P0/P1 incidents. This is a process gate, not a code test. Document the canary checklist in the rollout runbook. |
|
||||
|
||||
**Blocking:** 6.1, 6.2, 6.3 are hard blockers. 6.4 is a soft blocker. 6.5 is a process requirement.
|
||||
|
||||
---
|
||||
|
||||
## Non-Goals for Phase 6
|
||||
|
||||
These are explicitly out of scope. Do not implement them as part of Phase 6 acceptance.
|
||||
|
||||
- **Full reflection pipeline** (Phase 7) -- thread post-mortem analysis and lesson extraction
|
||||
- **WASM/Docker thread isolation** (Phase 8) -- running threads in sandboxed containers
|
||||
- **Performance optimization beyond parity** -- v2 should match v1, not beat it (improvements are welcome but not required)
|
||||
- **Mission system** -- `Mission` type is defined but not wired up
|
||||
- **Provenance tracking / taint analysis** -- structs exist but enforcement is Phase 7
|
||||
- **Two-phase commit for Financial effects** -- design is documented in Phase 6 spec, but implementation may defer to Phase 7 if no Financial-effect tools exist yet
|
||||
- **Dual model routing** -- `LlmBridgeAdapter` should support it structurally but it is not a Phase 6 acceptance criterion
|
||||
|
||||
---
|
||||
|
||||
## Verification Plan
|
||||
|
||||
### Automated Tests (CI-blocking)
|
||||
|
||||
```bash
|
||||
# 1. Engine unit tests (existing)
|
||||
cargo test -p ironclaw_engine
|
||||
|
||||
# 2. Bridge adapter tests (new)
|
||||
cargo test -p ironclaw -- bridge
|
||||
|
||||
# 3. Integration tests with both backends
|
||||
cargo test --features integration -- engine_v2
|
||||
cargo test -- engine_v2 # libSQL path
|
||||
|
||||
# 4. E2E tests with v2 engine
|
||||
cd tests/e2e && ENGINE_V2=true pytest
|
||||
|
||||
# 5. Behavioral equivalence diff tests
|
||||
cargo test -- behavioral_equivalence
|
||||
|
||||
# 6. Performance benchmarks (CI-reported, not CI-blocking)
|
||||
cargo bench -p ironclaw --bench step_latency
|
||||
cargo bench -p ironclaw_engine --bench monty_startup
|
||||
```
|
||||
|
||||
### Manual Review (PR-blocking)
|
||||
|
||||
- [ ] Security audit checklist (criterion 3.6) signed off by reviewer
|
||||
- [ ] Migration documentation (criterion 4.5) exists and reviewed
|
||||
- [ ] Canary runbook (criterion 6.5) exists
|
||||
|
||||
### Test Fixtures Required
|
||||
|
||||
| Fixture | Purpose | Location |
|
||||
|---------|---------|----------|
|
||||
| `trace_basic_conversation.json` | Multi-turn chat with tool calls | `tests/fixtures/engine_v2/` |
|
||||
| `trace_approval_flow.json` | Tool requiring approval -> approve -> complete | `tests/fixtures/engine_v2/` |
|
||||
| `trace_error_handling.json` | Invalid tool, malformed params, timeout | `tests/fixtures/engine_v2/` |
|
||||
| `trace_compaction.json` | 50-turn conversation triggering compaction | `tests/fixtures/engine_v2/` |
|
||||
| `trace_codeact.json` | CodeAct/Monty execution with tool dispatch | `tests/fixtures/engine_v2/` |
|
||||
|
||||
### Benchmark Harness Requirements
|
||||
|
||||
The step latency benchmark must:
|
||||
1. Use the same mock LLM (fixed response, configurable latency) for both engines
|
||||
2. Run in the same binary to eliminate process-level variance
|
||||
3. Report P50/P95/P99 with confidence intervals
|
||||
4. Run at least 1000 iterations per engine
|
||||
5. Warm up with 100 iterations before measurement
|
||||
6. Be added to CI as a reporting job (not a gate) with regression alerts at +15%
|
||||
|
||||
### Definition of Done
|
||||
|
||||
Phase 6 is complete when:
|
||||
1. All hard-blocker criteria pass in CI
|
||||
2. All soft-blocker criteria either pass or have filed issues with mitigation plans
|
||||
3. Security review checklist is signed off
|
||||
4. Migration documentation exists
|
||||
5. Canary runbook exists
|
||||
6. PR is approved by at least one reviewer who has read this document
|
||||
@@ -1,139 +0,0 @@
|
||||
#!/usr/bin/env bash
|
||||
set -euo pipefail
|
||||
|
||||
usage() {
|
||||
cat <<'EOF'
|
||||
Usage: scripts/monitor-prs.sh [--repo owner/name] [--author login]
|
||||
|
||||
Shows open PRs for the author with:
|
||||
- review decision
|
||||
- latest review summary
|
||||
- failing or pending checks
|
||||
|
||||
Defaults:
|
||||
- repo: current gitHub repo from `gh repo view`
|
||||
- author: currently authenticated GitHub user from `gh api user`
|
||||
EOF
|
||||
}
|
||||
|
||||
repo=""
|
||||
author=""
|
||||
|
||||
while [ $# -gt 0 ]; do
|
||||
case "$1" in
|
||||
--repo)
|
||||
repo="${2:-}"
|
||||
shift 2
|
||||
;;
|
||||
--author)
|
||||
author="${2:-}"
|
||||
shift 2
|
||||
;;
|
||||
-h|--help)
|
||||
usage
|
||||
exit 0
|
||||
;;
|
||||
*)
|
||||
echo "Unknown argument: $1" >&2
|
||||
usage >&2
|
||||
exit 1
|
||||
;;
|
||||
esac
|
||||
done
|
||||
|
||||
if ! command -v gh >/dev/null 2>&1; then
|
||||
echo "gh CLI is required" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
if ! command -v jq >/dev/null 2>&1; then
|
||||
echo "jq is required" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
if [ -z "$repo" ]; then
|
||||
repo="$(gh repo view --json nameWithOwner -q .nameWithOwner)"
|
||||
fi
|
||||
|
||||
if [ -z "$author" ]; then
|
||||
author="$(gh api user -q .login)"
|
||||
fi
|
||||
|
||||
json_fields="number,title,url,headRefName,reviewDecision,latestReviews,statusCheckRollup"
|
||||
prs="$(gh pr list --repo "$repo" --author "$author" --state open --limit 100 --json "$json_fields")"
|
||||
|
||||
count="$(printf '%s' "$prs" | jq 'length')"
|
||||
echo "Open PRs for $author in $repo: $count"
|
||||
echo
|
||||
|
||||
if [ "$count" -eq 0 ]; then
|
||||
exit 0
|
||||
fi
|
||||
|
||||
printf '%s' "$prs" | jq -r '
|
||||
def check_name:
|
||||
.name // .context // .workflowName // "unknown-check";
|
||||
|
||||
def failing_checks:
|
||||
[.statusCheckRollup[]?
|
||||
| select(.status == "COMPLETED" and (.conclusion // .state // "") != "SUCCESS")
|
||||
| {
|
||||
name: check_name,
|
||||
workflow: (.workflowName // ""),
|
||||
url: (.detailsUrl // "")
|
||||
}];
|
||||
|
||||
def pending_checks:
|
||||
[.statusCheckRollup[]?
|
||||
| select(.status != "COMPLETED")
|
||||
| {
|
||||
name: check_name,
|
||||
workflow: (.workflowName // ""),
|
||||
url: (.detailsUrl // "")
|
||||
}];
|
||||
|
||||
.[]
|
||||
| . as $pr
|
||||
| failing_checks as $failing
|
||||
| pending_checks as $pending
|
||||
| [
|
||||
("#" + (.number | tostring) + " " + .title),
|
||||
(" Branch: " + .headRefName),
|
||||
(" URL: " + .url),
|
||||
(" Review: " + (.reviewDecision // "UNKNOWN")),
|
||||
(
|
||||
if (.latestReviews | length) > 0 then
|
||||
" Latest review: "
|
||||
+ .latestReviews[0].state
|
||||
+ " by "
|
||||
+ .latestReviews[0].author.login
|
||||
+ " at "
|
||||
+ .latestReviews[0].submittedAt
|
||||
else
|
||||
" Latest review: none"
|
||||
end
|
||||
),
|
||||
(" Checks: " + ($failing | length | tostring) + " failing, "
|
||||
+ ($pending | length | tostring) + " pending"),
|
||||
(
|
||||
if ($failing | length) > 0 then
|
||||
($failing[] | " FAIL: " + .name
|
||||
+ (if .workflow != "" then " [" + .workflow + "]" else "" end)
|
||||
+ (if .url != "" then " -> " + .url else "" end))
|
||||
else
|
||||
" FAIL: none"
|
||||
end
|
||||
),
|
||||
(
|
||||
if ($pending | length) > 0 then
|
||||
($pending[] | " PENDING: " + .name
|
||||
+ (if .workflow != "" then " [" + .workflow + "]" else "" end)
|
||||
+ (if .url != "" then " -> " + .url else "" end))
|
||||
else
|
||||
" PENDING: none"
|
||||
end
|
||||
)
|
||||
]
|
||||
| .[]
|
||||
, ""
|
||||
'
|
||||
@@ -731,7 +731,7 @@ impl Agent {
|
||||
{
|
||||
use crate::agent::session::Thread;
|
||||
let mut sess = session.lock().await;
|
||||
let thread = Thread::with_id(id, sess.id, None);
|
||||
let thread = Thread::with_id(id, sess.id);
|
||||
sess.active_thread = Some(id);
|
||||
sess.threads.entry(id).or_insert(thread);
|
||||
}
|
||||
@@ -1010,8 +1010,13 @@ impl Agent {
|
||||
thread_id = %external_thread_id,
|
||||
"Hydrating thread from DB"
|
||||
);
|
||||
if let Some(rejection) = self.maybe_hydrate_thread(message, external_thread_id).await {
|
||||
return Ok(Some(format!("Error: {}", rejection)));
|
||||
match self.maybe_hydrate_thread(message, external_thread_id).await {
|
||||
Err(rejection) => {
|
||||
return Ok(Some(format!("Error: {}", rejection)));
|
||||
}
|
||||
Ok(_) => {
|
||||
// Ready, Skipped, or NotFound — all proceed to resolve_thread
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -319,7 +319,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_format_turns() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
thread.start_turn("Hello");
|
||||
thread.complete_turn("Hi there");
|
||||
thread.start_turn("How are you?");
|
||||
@@ -351,7 +351,7 @@ mod tests {
|
||||
/// Helper: build a thread with `n` completed turns.
|
||||
/// Turn `i` has user_input "msg-{i}" and response "resp-{i}".
|
||||
fn make_thread(n: usize) -> Thread {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
for i in 0..n {
|
||||
thread.start_turn(format!("msg-{}", i));
|
||||
thread.complete_turn(format!("resp-{}", i));
|
||||
@@ -457,7 +457,7 @@ mod tests {
|
||||
async fn test_compact_truncate_empty_turns() {
|
||||
let llm = Arc::new(StubLlm::new("unused"));
|
||||
let compactor = make_compactor(llm);
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
assert!(thread.turns.is_empty());
|
||||
|
||||
let result = compactor
|
||||
@@ -698,7 +698,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_format_turns_for_storage_with_tool_calls() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
thread.start_turn("Search for X");
|
||||
// Record a tool call on the current turn
|
||||
if let Some(turn) = thread.turns.last_mut() {
|
||||
@@ -719,7 +719,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_format_turns_for_storage_incomplete_turn() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
thread.start_turn("In progress message");
|
||||
// Don't complete the turn
|
||||
|
||||
|
||||
@@ -2140,7 +2140,7 @@ mod tests {
|
||||
// Initialize a thread in the session so the loop can record tool calls.
|
||||
let thread_id = {
|
||||
let mut sess = session.lock().await;
|
||||
sess.create_thread("test").id
|
||||
sess.create_thread().id
|
||||
};
|
||||
|
||||
let message = IncomingMessage::new("test", "test-user", "do something");
|
||||
@@ -2245,7 +2245,7 @@ mod tests {
|
||||
let session = Arc::new(Mutex::new(Session::new("test-user")));
|
||||
let thread_id = {
|
||||
let mut sess = session.lock().await;
|
||||
sess.create_thread("test").id
|
||||
sess.create_thread().id
|
||||
};
|
||||
|
||||
let message = IncomingMessage::new("test", "test-user", "keep calling tools");
|
||||
|
||||
+78
-100
@@ -68,8 +68,8 @@ impl Session {
|
||||
}
|
||||
|
||||
/// Create a new thread in this session.
|
||||
pub fn create_thread(&mut self, channel: &str) -> &mut Thread {
|
||||
let thread = Thread::new(self.id, Some(channel));
|
||||
pub fn create_thread(&mut self) -> &mut Thread {
|
||||
let thread = Thread::new(self.id);
|
||||
let thread_id = thread.id;
|
||||
self.active_thread = Some(thread_id);
|
||||
self.last_active_at = Utc::now();
|
||||
@@ -87,9 +87,9 @@ impl Session {
|
||||
}
|
||||
|
||||
/// Get or create the active thread.
|
||||
pub fn get_or_create_thread(&mut self, channel: &str) -> &mut Thread {
|
||||
pub fn get_or_create_thread(&mut self) -> &mut Thread {
|
||||
match self.active_thread {
|
||||
None => self.create_thread(channel),
|
||||
None => self.create_thread(),
|
||||
Some(id) => {
|
||||
if self.threads.contains_key(&id) {
|
||||
// Entry existence confirmed by contains_key above.
|
||||
@@ -100,7 +100,7 @@ impl Session {
|
||||
} else {
|
||||
// Stale active_thread ID: create a new thread, which
|
||||
// updates self.active_thread to the new thread's ID.
|
||||
self.create_thread(channel)
|
||||
self.create_thread()
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -225,9 +225,6 @@ pub struct Thread {
|
||||
/// Messages queued while the thread was processing a turn.
|
||||
#[serde(default, skip_serializing_if = "VecDeque::is_empty")]
|
||||
pub pending_messages: VecDeque<String>,
|
||||
/// Channel that created this thread (for approval authorization).
|
||||
#[serde(default)]
|
||||
pub source_channel: Option<String>,
|
||||
}
|
||||
|
||||
/// Maximum number of messages that can be queued while a thread is processing.
|
||||
@@ -238,7 +235,7 @@ pub const MAX_PENDING_MESSAGES: usize = 10;
|
||||
|
||||
impl Thread {
|
||||
/// Create a new thread.
|
||||
pub fn new(session_id: Uuid, source_channel: Option<&str>) -> Self {
|
||||
pub fn new(session_id: Uuid) -> Self {
|
||||
let now = Utc::now();
|
||||
Self {
|
||||
id: Uuid::new_v4(),
|
||||
@@ -251,12 +248,11 @@ impl Thread {
|
||||
pending_approval: None,
|
||||
pending_auth: None,
|
||||
pending_messages: VecDeque::new(),
|
||||
source_channel: source_channel.map(String::from),
|
||||
}
|
||||
}
|
||||
|
||||
/// Create a thread with a specific ID (for DB hydration).
|
||||
pub fn with_id(id: Uuid, session_id: Uuid, source_channel: Option<&str>) -> Self {
|
||||
pub fn with_id(id: Uuid, session_id: Uuid) -> Self {
|
||||
let now = Utc::now();
|
||||
Self {
|
||||
id,
|
||||
@@ -269,7 +265,6 @@ impl Thread {
|
||||
pending_approval: None,
|
||||
pending_auth: None,
|
||||
pending_messages: VecDeque::new(),
|
||||
source_channel: source_channel.map(String::from),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -701,13 +696,13 @@ mod tests {
|
||||
let mut session = Session::new("user-123");
|
||||
assert!(session.active_thread.is_none());
|
||||
|
||||
session.create_thread("test");
|
||||
session.create_thread();
|
||||
assert!(session.active_thread.is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_thread_turns() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("Hello");
|
||||
assert_eq!(thread.state, ThreadState::Processing);
|
||||
@@ -720,7 +715,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_thread_messages() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("First message");
|
||||
thread.complete_turn("First response");
|
||||
@@ -743,7 +738,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_restore_from_messages() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// First add some turns
|
||||
thread.start_turn("Original message");
|
||||
@@ -769,7 +764,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_restore_from_messages_incomplete_turn() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Messages with incomplete last turn (no assistant response)
|
||||
let messages = vec![
|
||||
@@ -788,7 +783,7 @@ mod tests {
|
||||
#[test]
|
||||
fn test_enter_auth_mode() {
|
||||
let before = Utc::now();
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
assert!(thread.pending_auth.is_none());
|
||||
|
||||
thread.enter_auth_mode("telegram".to_string());
|
||||
@@ -801,7 +796,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_take_pending_auth() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
thread.enter_auth_mode("notion".to_string());
|
||||
|
||||
let pending = thread.take_pending_auth();
|
||||
@@ -816,7 +811,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_pending_auth_serialization() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
thread.enter_auth_mode("openai".to_string());
|
||||
|
||||
let json = serde_json::to_string(&thread).expect("should serialize");
|
||||
@@ -846,7 +841,7 @@ mod tests {
|
||||
#[test]
|
||||
fn test_pending_auth_default_none() {
|
||||
// Deserialization of old data without pending_auth should default to None
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
thread.pending_auth = None;
|
||||
let json = serde_json::to_string(&thread).expect("serialize");
|
||||
|
||||
@@ -860,7 +855,7 @@ mod tests {
|
||||
fn test_thread_with_id() {
|
||||
let specific_id = Uuid::new_v4();
|
||||
let session_id = Uuid::new_v4();
|
||||
let thread = Thread::with_id(specific_id, session_id, None);
|
||||
let thread = Thread::with_id(specific_id, session_id);
|
||||
|
||||
assert_eq!(thread.id, specific_id);
|
||||
assert_eq!(thread.session_id, session_id);
|
||||
@@ -872,7 +867,7 @@ mod tests {
|
||||
fn test_thread_with_id_restore_messages() {
|
||||
let thread_id = Uuid::new_v4();
|
||||
let session_id = Uuid::new_v4();
|
||||
let mut thread = Thread::with_id(thread_id, session_id, None);
|
||||
let mut thread = Thread::with_id(thread_id, session_id);
|
||||
|
||||
let messages = vec![
|
||||
ChatMessage::user("Hello from DB"),
|
||||
@@ -891,7 +886,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_restore_from_messages_empty() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Add a turn first, then restore with empty vec
|
||||
thread.start_turn("hello");
|
||||
@@ -907,7 +902,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_restore_from_messages_only_assistant_messages() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Only assistant messages (no user messages to anchor turns)
|
||||
let messages = vec![
|
||||
@@ -924,7 +919,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_restore_from_messages_multiple_user_messages_in_a_row() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Two user messages with no assistant response between them
|
||||
let messages = vec![
|
||||
@@ -951,8 +946,8 @@ mod tests {
|
||||
fn test_thread_switch() {
|
||||
let mut session = Session::new("user-1");
|
||||
|
||||
let t1_id = session.create_thread("test").id;
|
||||
let t2_id = session.create_thread("test").id;
|
||||
let t1_id = session.create_thread().id;
|
||||
let t2_id = session.create_thread().id;
|
||||
|
||||
// After creating two threads, active should be the last one
|
||||
assert_eq!(session.active_thread, Some(t2_id));
|
||||
@@ -972,8 +967,8 @@ mod tests {
|
||||
fn test_get_or_create_thread_idempotent() {
|
||||
let mut session = Session::new("user-1");
|
||||
|
||||
let tid1 = session.get_or_create_thread("test").id;
|
||||
let tid2 = session.get_or_create_thread("test").id;
|
||||
let tid1 = session.get_or_create_thread().id;
|
||||
let tid2 = session.get_or_create_thread().id;
|
||||
|
||||
// Should return the same thread (not create a new one each time)
|
||||
assert_eq!(tid1, tid2);
|
||||
@@ -982,7 +977,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_truncate_turns() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
for i in 0..5 {
|
||||
thread.start_turn(format!("msg-{}", i));
|
||||
@@ -1006,7 +1001,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_truncate_turns_noop_when_fewer() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("only one");
|
||||
thread.complete_turn("response");
|
||||
@@ -1018,7 +1013,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_thread_interrupt_and_resume() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("do something");
|
||||
assert_eq!(thread.state, ThreadState::Processing);
|
||||
@@ -1036,7 +1031,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_resume_only_from_interrupted() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Idle thread: resume should be a no-op
|
||||
assert_eq!(thread.state, ThreadState::Idle);
|
||||
@@ -1052,7 +1047,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_turn_fail() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("risky operation");
|
||||
thread.fail_turn("connection timed out");
|
||||
@@ -1068,7 +1063,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_messages_with_incomplete_last_turn() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("first");
|
||||
thread.complete_turn("first reply");
|
||||
@@ -1084,7 +1079,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_thread_serialization_round_trip() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("hello");
|
||||
thread.complete_turn("world");
|
||||
@@ -1102,7 +1097,7 @@ mod tests {
|
||||
#[test]
|
||||
fn test_session_serialization_round_trip() {
|
||||
let mut session = Session::new("user-ser");
|
||||
session.create_thread("test");
|
||||
session.create_thread();
|
||||
session.auto_approve_tool("echo");
|
||||
|
||||
let json = serde_json::to_string(&session).unwrap();
|
||||
@@ -1140,7 +1135,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_turn_number_increments() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Before any turns, turn_number() is 1 (1-indexed for display)
|
||||
assert_eq!(thread.turn_number(), 1);
|
||||
@@ -1155,7 +1150,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_complete_turn_on_empty_thread() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Completing a turn when there are no turns should be a safe no-op
|
||||
thread.complete_turn("phantom response");
|
||||
@@ -1165,7 +1160,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_fail_turn_on_empty_thread() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Failing a turn when there are no turns should be a safe no-op
|
||||
thread.fail_turn("phantom error");
|
||||
@@ -1175,7 +1170,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_pending_approval_flow() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
let approval = PendingApproval {
|
||||
request_id: Uuid::new_v4(),
|
||||
@@ -1202,7 +1197,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_clear_pending_approval() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
let approval = PendingApproval {
|
||||
request_id: Uuid::new_v4(),
|
||||
@@ -1231,7 +1226,7 @@ mod tests {
|
||||
assert!(session.active_thread().is_none());
|
||||
assert!(session.active_thread_mut().is_none());
|
||||
|
||||
let tid = session.create_thread("test").id;
|
||||
let tid = session.create_thread().id;
|
||||
|
||||
assert!(session.active_thread().is_some());
|
||||
assert_eq!(session.active_thread().unwrap().id, tid);
|
||||
@@ -1248,7 +1243,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_messages_includes_tool_calls() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("Search for X");
|
||||
{
|
||||
@@ -1280,7 +1275,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_messages_multiple_tool_calls_per_turn() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("Do two things");
|
||||
{
|
||||
@@ -1307,7 +1302,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_restore_from_messages_with_tool_calls() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Build a message sequence with tool calls
|
||||
let tc = ToolCall {
|
||||
@@ -1338,7 +1333,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_restore_from_messages_with_tool_error() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
let tc = ToolCall {
|
||||
id: "call_0".to_string(),
|
||||
@@ -1368,7 +1363,7 @@ mod tests {
|
||||
fn test_messages_round_trip_with_tools() {
|
||||
// Build a thread with tool calls, get messages(), restore, get messages() again
|
||||
// The two message sequences should be equivalent.
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("Do search");
|
||||
{
|
||||
@@ -1381,7 +1376,7 @@ mod tests {
|
||||
let messages_original = thread.messages();
|
||||
|
||||
// Restore into a new thread
|
||||
let mut thread2 = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread2 = Thread::new(Uuid::new_v4());
|
||||
thread2.restore_from_messages(messages_original.clone());
|
||||
|
||||
let messages_restored = thread2.messages();
|
||||
@@ -1403,7 +1398,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_restore_multi_stage_tool_calls() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
let tc1 = ToolCall {
|
||||
id: "call_a".to_string(),
|
||||
@@ -1444,7 +1439,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_messages_truncates_large_tool_results() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
thread.start_turn("Read big file");
|
||||
{
|
||||
@@ -1467,11 +1462,13 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_thread_message_queue() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Queue is initially empty
|
||||
assert!(thread.pending_messages.is_empty());
|
||||
assert!(thread.take_pending_message().is_none());
|
||||
|
||||
// Queue messages and verify FIFO ordering
|
||||
assert!(thread.queue_message("first".to_string()));
|
||||
assert!(thread.queue_message("second".to_string()));
|
||||
assert!(thread.queue_message("third".to_string()));
|
||||
@@ -1482,14 +1479,17 @@ mod tests {
|
||||
assert_eq!(thread.take_pending_message(), Some("third".to_string()));
|
||||
assert!(thread.take_pending_message().is_none());
|
||||
|
||||
// Fill to capacity — all 10 should succeed
|
||||
for i in 0..MAX_PENDING_MESSAGES {
|
||||
assert!(thread.queue_message(format!("msg-{}", i)));
|
||||
}
|
||||
assert_eq!(thread.pending_messages.len(), MAX_PENDING_MESSAGES);
|
||||
|
||||
// 11th message rejected by queue_message itself
|
||||
assert!(!thread.queue_message("overflow".to_string()));
|
||||
assert_eq!(thread.pending_messages.len(), MAX_PENDING_MESSAGES);
|
||||
|
||||
// Drain and verify order
|
||||
for i in 0..MAX_PENDING_MESSAGES {
|
||||
assert_eq!(thread.take_pending_message(), Some(format!("msg-{}", i)));
|
||||
}
|
||||
@@ -1498,11 +1498,13 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_thread_message_queue_serialization() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Empty queue should not appear in serialization (skip_serializing_if)
|
||||
let json = serde_json::to_string(&thread).unwrap();
|
||||
assert!(!json.contains("pending_messages"));
|
||||
|
||||
// Non-empty queue should serialize and deserialize
|
||||
thread.queue_message("queued msg".to_string());
|
||||
let json = serde_json::to_string(&thread).unwrap();
|
||||
assert!(json.contains("pending_messages"));
|
||||
@@ -1515,9 +1517,11 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_thread_message_queue_default_on_old_data() {
|
||||
let thread = Thread::new(Uuid::new_v4(), None);
|
||||
// Deserialization of old data without pending_messages should default to empty
|
||||
let thread = Thread::new(Uuid::new_v4());
|
||||
let json = serde_json::to_string(&thread).unwrap();
|
||||
|
||||
// The field is absent (skip_serializing_if), simulating old data
|
||||
assert!(!json.contains("pending_messages"));
|
||||
let restored: Thread = serde_json::from_str(&json).unwrap();
|
||||
assert!(restored.pending_messages.is_empty());
|
||||
@@ -1525,15 +1529,18 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_interrupt_clears_pending_messages() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Start a turn so there's something to interrupt
|
||||
thread.start_turn("initial input");
|
||||
|
||||
// Queue several messages while "processing"
|
||||
thread.queue_message("queued-1".to_string());
|
||||
thread.queue_message("queued-2".to_string());
|
||||
thread.queue_message("queued-3".to_string());
|
||||
assert_eq!(thread.pending_messages.len(), 3);
|
||||
|
||||
// Interrupt should clear the queue
|
||||
thread.interrupt();
|
||||
assert!(thread.pending_messages.is_empty());
|
||||
assert_eq!(thread.state, ThreadState::Interrupted);
|
||||
@@ -1541,22 +1548,27 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_thread_state_idle_after_full_drain() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Simulate a full drain cycle: start turn, queue messages, complete turn,
|
||||
// then drain all queued messages as a single merged turn (#259).
|
||||
thread.start_turn("turn 1");
|
||||
assert_eq!(thread.state, ThreadState::Processing);
|
||||
|
||||
thread.queue_message("queued-a".to_string());
|
||||
thread.queue_message("queued-b".to_string());
|
||||
|
||||
// Complete the turn (simulates process_user_input finishing)
|
||||
thread.complete_turn("response 1");
|
||||
assert_eq!(thread.state, ThreadState::Idle);
|
||||
|
||||
// Drain: merge all queued messages and process as a single turn
|
||||
let merged = thread.drain_pending_messages().unwrap();
|
||||
assert_eq!(merged, "queued-a\nqueued-b");
|
||||
thread.start_turn(&merged);
|
||||
thread.complete_turn("response for merged");
|
||||
|
||||
// Queue is fully drained, thread is idle
|
||||
assert!(thread.drain_pending_messages().is_none());
|
||||
assert!(thread.pending_messages.is_empty());
|
||||
assert_eq!(thread.state, ThreadState::Idle);
|
||||
@@ -1564,10 +1576,12 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_drain_pending_messages_merges_with_newlines() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Empty queue returns None
|
||||
assert!(thread.drain_pending_messages().is_none());
|
||||
|
||||
// Single message returned as-is (no trailing newline)
|
||||
thread.queue_message("only one".to_string());
|
||||
assert_eq!(
|
||||
thread.drain_pending_messages(),
|
||||
@@ -1575,6 +1589,7 @@ mod tests {
|
||||
);
|
||||
assert!(thread.pending_messages.is_empty());
|
||||
|
||||
// Multiple messages joined with newlines
|
||||
thread.queue_message("hey".to_string());
|
||||
thread.queue_message("can you check the server".to_string());
|
||||
thread.queue_message("it started 10 min ago".to_string());
|
||||
@@ -1584,62 +1599,25 @@ mod tests {
|
||||
);
|
||||
assert!(thread.pending_messages.is_empty());
|
||||
|
||||
// Queue is empty after drain
|
||||
assert!(thread.drain_pending_messages().is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_requeue_drained_preserves_content_at_front() {
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
|
||||
// Re-queue into empty queue
|
||||
thread.requeue_drained("failed batch".to_string());
|
||||
assert_eq!(thread.pending_messages.len(), 1);
|
||||
assert_eq!(thread.pending_messages[0], "failed batch");
|
||||
|
||||
// New messages go behind the re-queued content
|
||||
thread.queue_message("new msg".to_string());
|
||||
assert_eq!(thread.pending_messages.len(), 2);
|
||||
|
||||
// Drain should return re-queued content first (front of queue)
|
||||
let merged = thread.drain_pending_messages().unwrap();
|
||||
assert_eq!(merged, "failed batch\nnew msg");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_thread_new_stores_source_channel() {
|
||||
let thread = Thread::new(Uuid::new_v4(), Some("gateway"));
|
||||
assert_eq!(thread.source_channel.as_deref(), Some("gateway"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_thread_new_none_channel() {
|
||||
let thread = Thread::new(Uuid::new_v4(), None);
|
||||
assert!(thread.source_channel.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_thread_with_id_stores_source_channel() {
|
||||
let thread = Thread::with_id(Uuid::new_v4(), Uuid::new_v4(), Some("http"));
|
||||
assert_eq!(thread.source_channel.as_deref(), Some("http"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_create_thread_sets_source_channel() {
|
||||
let mut session = Session::new("user-chan");
|
||||
let thread_id = session.create_thread("gateway").id;
|
||||
let thread = session.threads.get(&thread_id).unwrap();
|
||||
assert_eq!(thread.source_channel.as_deref(), Some("gateway"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_source_channel_serde_backcompat() {
|
||||
let json = r#"{
|
||||
"id": "00000000-0000-0000-0000-000000000001",
|
||||
"session_id": "00000000-0000-0000-0000-000000000002",
|
||||
"state": "Idle",
|
||||
"turns": [],
|
||||
"created_at": "2025-01-01T00:00:00Z",
|
||||
"updated_at": "2025-01-01T00:00:00Z",
|
||||
"metadata": null
|
||||
}"#;
|
||||
let thread: Thread = serde_json::from_str(json).unwrap();
|
||||
assert!(thread.source_channel.is_none());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -167,7 +167,7 @@ impl SessionManager {
|
||||
// Create new thread (always create a new one for a new key)
|
||||
let thread_id = {
|
||||
let mut sess = session.lock().await;
|
||||
let thread = sess.create_thread(channel);
|
||||
let thread = sess.create_thread();
|
||||
thread.id
|
||||
};
|
||||
|
||||
@@ -443,7 +443,7 @@ mod tests {
|
||||
let session = Arc::new(Mutex::new(Session::new("user-hydrate")));
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
let thread = Thread::with_id(thread_id, sess.id, None);
|
||||
let thread = Thread::with_id(thread_id, sess.id);
|
||||
sess.threads.insert(thread_id, thread);
|
||||
sess.active_thread = Some(thread_id);
|
||||
}
|
||||
@@ -567,7 +567,7 @@ mod tests {
|
||||
// Simulate hydration: create thread with a known UUID
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
let thread = Thread::with_id(known_uuid, session_id, None);
|
||||
let thread = Thread::with_id(known_uuid, session_id);
|
||||
sess.threads.insert(known_uuid, thread);
|
||||
}
|
||||
|
||||
@@ -594,7 +594,7 @@ mod tests {
|
||||
let session = Arc::new(Mutex::new(Session::new("user-idem")));
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
let thread = Thread::with_id(tid, sess.id, None);
|
||||
let thread = Thread::with_id(tid, sess.id);
|
||||
sess.threads.insert(tid, thread);
|
||||
}
|
||||
|
||||
@@ -623,7 +623,7 @@ mod tests {
|
||||
let session = Arc::new(Mutex::new(Session::new("user-undo")));
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
let thread = Thread::with_id(tid, sess.id, None);
|
||||
let thread = Thread::with_id(tid, sess.id);
|
||||
sess.threads.insert(tid, thread);
|
||||
}
|
||||
|
||||
@@ -647,7 +647,7 @@ mod tests {
|
||||
let session = Arc::new(Mutex::new(Session::new("user-new")));
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
let thread = Thread::with_id(tid, sess.id, None);
|
||||
let thread = Thread::with_id(tid, sess.id);
|
||||
sess.threads.insert(tid, thread);
|
||||
}
|
||||
|
||||
@@ -755,7 +755,7 @@ mod tests {
|
||||
let session = Arc::new(Mutex::new(Session::new("user-cross")));
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
let thread = Thread::with_id(tid, sess.id, None);
|
||||
let thread = Thread::with_id(tid, sess.id);
|
||||
sess.threads.insert(tid, thread);
|
||||
}
|
||||
|
||||
@@ -782,7 +782,7 @@ mod tests {
|
||||
let session = Arc::new(Mutex::new(Session::new("user-cross")));
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
let thread = Thread::with_id(tid, sess.id, None);
|
||||
let thread = Thread::with_id(tid, sess.id);
|
||||
sess.threads.insert(tid, thread);
|
||||
}
|
||||
|
||||
@@ -921,7 +921,7 @@ mod tests {
|
||||
let session = Arc::new(Mutex::new(Session::new("user-direct")));
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
let thread = Thread::with_id(tid, sess.id, None);
|
||||
let thread = Thread::with_id(tid, sess.id);
|
||||
sess.threads.insert(tid, thread);
|
||||
}
|
||||
{
|
||||
@@ -947,23 +947,4 @@ mod tests {
|
||||
"should have exactly 1 thread, not a duplicate"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_thread_stores_source_channel() {
|
||||
let manager = SessionManager::new();
|
||||
let (session, thread_id) = manager
|
||||
.resolve_thread("user1", "gateway", Some("ext-1"))
|
||||
.await;
|
||||
let sess = session.lock().await;
|
||||
let thread = sess.threads.get(&thread_id).unwrap();
|
||||
assert_eq!(thread.source_channel.as_deref(), Some("gateway"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_different_channels_get_different_threads() {
|
||||
let manager = SessionManager::new();
|
||||
let (_, tid1) = manager.resolve_thread("user1", "gateway", None).await;
|
||||
let (_, tid2) = manager.resolve_thread("user1", "web", None).await;
|
||||
assert_ne!(tid1, tid2);
|
||||
}
|
||||
}
|
||||
|
||||
+170
-54
@@ -25,6 +25,25 @@ use crate::tools::redact_params;
|
||||
|
||||
const FORGED_THREAD_ID_ERROR: &str = "Invalid or unauthorized thread ID.";
|
||||
|
||||
/// Result of attempting to hydrate a thread from the database.
|
||||
///
|
||||
/// Distinguishes between a thread that was fully hydrated into the session
|
||||
/// (messages loaded, thread registered) and one where we recognised the UUID
|
||||
/// but skipped hydration (e.g. already present in memory, or ownership could
|
||||
/// not be verified on a non-gateway channel).
|
||||
#[derive(Debug)]
|
||||
#[allow(dead_code)] // Inner UUIDs are part of the API contract for future callers
|
||||
pub(super) enum HydrationResult {
|
||||
/// Thread hydrated and available in `sess.threads` / `thread_map`.
|
||||
Ready(Uuid),
|
||||
/// UUID is known but hydration was intentionally skipped. The thread may
|
||||
/// already be in memory, or the caller is on a channel that does not
|
||||
/// require pre-existing threads so we fall through to `resolve_thread`.
|
||||
Skipped(Uuid),
|
||||
/// The external thread ID was not a valid UUID — nothing to hydrate.
|
||||
NotFound,
|
||||
}
|
||||
|
||||
fn requires_preexisting_uuid_thread(channel: &str) -> bool {
|
||||
// Gateway-style channels send server-issued conversation UUIDs.
|
||||
// Unknown UUIDs should be rejected instead of silently creating a new thread.
|
||||
@@ -41,15 +60,24 @@ impl Agent {
|
||||
/// even when the conversation has zero messages (e.g. a brand-new
|
||||
/// assistant thread). Without this, `resolve_thread` would mint a
|
||||
/// fresh UUID and all messages would land in the wrong conversation.
|
||||
///
|
||||
/// Returns [`HydrationResult::Ready`] when the thread was fully loaded
|
||||
/// into the session, [`HydrationResult::Skipped`] when the UUID was
|
||||
/// recognised but hydration was not performed (already in memory, or
|
||||
/// ownership unverifiable on a non-gateway channel), and
|
||||
/// [`HydrationResult::NotFound`] when the external ID is not a UUID.
|
||||
///
|
||||
/// Returns `Err` only for hard rejections (forged / unauthorised thread
|
||||
/// ID on a gateway channel).
|
||||
pub(super) async fn maybe_hydrate_thread(
|
||||
&self,
|
||||
message: &IncomingMessage,
|
||||
external_thread_id: &str,
|
||||
) -> Option<String> {
|
||||
) -> Result<HydrationResult, String> {
|
||||
// Only hydrate UUID-shaped thread IDs (web gateway uses UUIDs)
|
||||
let thread_uuid = match Uuid::parse_str(external_thread_id) {
|
||||
Ok(id) => id,
|
||||
Err(_) => return None,
|
||||
Err(_) => return Ok(HydrationResult::NotFound),
|
||||
};
|
||||
|
||||
// Check if already in memory
|
||||
@@ -60,7 +88,7 @@ impl Agent {
|
||||
{
|
||||
let sess = session.lock().await;
|
||||
if sess.threads.contains_key(&thread_uuid) {
|
||||
return None;
|
||||
return Ok(HydrationResult::Skipped(thread_uuid));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -83,9 +111,9 @@ impl Agent {
|
||||
e
|
||||
);
|
||||
if requires_preexisting_uuid_thread(&message.channel) {
|
||||
return Some(FORGED_THREAD_ID_ERROR.to_string());
|
||||
return Err(FORGED_THREAD_ID_ERROR.to_string());
|
||||
}
|
||||
return None;
|
||||
return Ok(HydrationResult::Skipped(thread_uuid));
|
||||
}
|
||||
};
|
||||
if !owned {
|
||||
@@ -99,9 +127,9 @@ impl Agent {
|
||||
e
|
||||
);
|
||||
if requires_preexisting_uuid_thread(&message.channel) {
|
||||
return Some(FORGED_THREAD_ID_ERROR.to_string());
|
||||
return Err(FORGED_THREAD_ID_ERROR.to_string());
|
||||
}
|
||||
return None;
|
||||
return Ok(HydrationResult::Skipped(thread_uuid));
|
||||
}
|
||||
};
|
||||
|
||||
@@ -113,7 +141,7 @@ impl Agent {
|
||||
exists,
|
||||
"Rejected message for unavailable thread id"
|
||||
);
|
||||
return Some(FORGED_THREAD_ID_ERROR.to_string());
|
||||
return Err(FORGED_THREAD_ID_ERROR.to_string());
|
||||
}
|
||||
|
||||
tracing::warn!(
|
||||
@@ -122,7 +150,7 @@ impl Agent {
|
||||
exists,
|
||||
"Skipped hydration for thread id not owned by sender"
|
||||
);
|
||||
return None;
|
||||
return Ok(HydrationResult::Skipped(thread_uuid));
|
||||
}
|
||||
|
||||
let db_messages = store
|
||||
@@ -141,8 +169,7 @@ impl Agent {
|
||||
sess.id
|
||||
};
|
||||
|
||||
let mut thread =
|
||||
crate::agent::session::Thread::with_id(thread_uuid, session_id, Some(&message.channel));
|
||||
let mut thread = crate::agent::session::Thread::with_id(thread_uuid, session_id);
|
||||
if !chat_messages.is_empty() {
|
||||
thread.restore_from_messages(chat_messages);
|
||||
}
|
||||
@@ -170,7 +197,7 @@ impl Agent {
|
||||
msg_count
|
||||
);
|
||||
|
||||
None
|
||||
Ok(HydrationResult::Ready(thread_uuid))
|
||||
}
|
||||
|
||||
pub(super) async fn process_user_input(
|
||||
@@ -955,29 +982,6 @@ impl Agent {
|
||||
approved: bool,
|
||||
always: bool,
|
||||
) -> Result<SubmissionResult, Error> {
|
||||
// Verify channel authorization: the approving channel must match the
|
||||
// thread's source channel, OR be the web gateway (trusted approval UI).
|
||||
{
|
||||
let sess = session.lock().await;
|
||||
if let Some(thread) = sess.threads.get(&thread_id) {
|
||||
let authorized = thread
|
||||
.source_channel
|
||||
.as_ref()
|
||||
.is_none_or(|src| src == &message.channel || message.channel == "web");
|
||||
if !authorized {
|
||||
tracing::warn!(
|
||||
%thread_id,
|
||||
source_channel = ?thread.source_channel,
|
||||
approval_channel = %message.channel,
|
||||
"Blocked cross-channel approval attempt"
|
||||
);
|
||||
return Ok(SubmissionResult::error(
|
||||
"approval not authorized for this channel",
|
||||
));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Get pending approval for this thread
|
||||
let pending = {
|
||||
let mut sess = session.lock().await;
|
||||
@@ -1437,8 +1441,20 @@ impl Agent {
|
||||
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
if let Some(thread) = sess.threads.get_mut(&thread_id) {
|
||||
thread.await_approval(new_pending);
|
||||
match sess.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
thread.await_approval(new_pending);
|
||||
}
|
||||
None => {
|
||||
tracing::error!(
|
||||
%thread_id,
|
||||
tool = %tool_name,
|
||||
"Thread disappeared while preparing approval request"
|
||||
);
|
||||
return Ok(SubmissionResult::error(
|
||||
"The conversation thread was pruned during processing. Some actions may have already been executed. Please check results before retrying.",
|
||||
));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1570,17 +1586,25 @@ impl Agent {
|
||||
);
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
if let Some(thread) = sess.threads.get_mut(&thread_id) {
|
||||
thread.clear_pending_approval();
|
||||
thread.complete_turn(&rejection);
|
||||
// User message already persisted at turn start; save rejection response
|
||||
self.persist_assistant_response(
|
||||
thread_id,
|
||||
&message.channel,
|
||||
&message.user_id,
|
||||
&rejection,
|
||||
)
|
||||
.await;
|
||||
match sess.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
thread.clear_pending_approval();
|
||||
thread.complete_turn(&rejection);
|
||||
// User message already persisted at turn start; save rejection response
|
||||
self.persist_assistant_response(
|
||||
thread_id,
|
||||
&message.channel,
|
||||
&message.user_id,
|
||||
&rejection,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
None => {
|
||||
tracing::warn!(
|
||||
%thread_id,
|
||||
"Thread disappeared during approval rejection — rejection not persisted"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1768,7 +1792,7 @@ impl Agent {
|
||||
.get_or_create_session(&message.user_id)
|
||||
.await;
|
||||
let mut sess = session.lock().await;
|
||||
let thread = sess.create_thread(&message.channel);
|
||||
let thread = sess.create_thread();
|
||||
let thread_id = thread.id;
|
||||
Ok(SubmissionResult::ok_with_message(format!(
|
||||
"New thread: {}",
|
||||
@@ -2059,7 +2083,7 @@ mod tests {
|
||||
|
||||
let session_id = Uuid::new_v4();
|
||||
let thread_id = Uuid::new_v4();
|
||||
let mut thread = Thread::with_id(thread_id, session_id, None);
|
||||
let mut thread = Thread::with_id(thread_id, session_id);
|
||||
|
||||
// Set thread to AwaitingApproval with a pending tool approval
|
||||
let pending = PendingApproval {
|
||||
@@ -2127,7 +2151,7 @@ mod tests {
|
||||
use crate::agent::session::{MAX_PENDING_MESSAGES, Thread, ThreadState};
|
||||
use uuid::Uuid;
|
||||
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
thread.start_turn("processing something");
|
||||
assert_eq!(thread.state, ThreadState::Processing);
|
||||
|
||||
@@ -2153,7 +2177,7 @@ mod tests {
|
||||
use crate::agent::session::{Thread, ThreadState};
|
||||
use uuid::Uuid;
|
||||
|
||||
let mut thread = Thread::new(Uuid::new_v4(), None);
|
||||
let mut thread = Thread::new(Uuid::new_v4());
|
||||
thread.start_turn("processing");
|
||||
|
||||
thread.queue_message("pending-1".to_string());
|
||||
@@ -2183,7 +2207,7 @@ mod tests {
|
||||
|
||||
let thread_id = Uuid::new_v4();
|
||||
let session_id = Uuid::new_v4();
|
||||
let mut thread = Thread::with_id(thread_id, session_id, None);
|
||||
let mut thread = Thread::with_id(thread_id, session_id);
|
||||
thread.start_turn("working");
|
||||
assert_eq!(thread.state, ThreadState::Processing);
|
||||
|
||||
@@ -2210,7 +2234,7 @@ mod tests {
|
||||
|
||||
let thread_id = Uuid::new_v4();
|
||||
let session_id = Uuid::new_v4();
|
||||
let mut thread = Thread::with_id(thread_id, session_id, None);
|
||||
let mut thread = Thread::with_id(thread_id, session_id);
|
||||
thread.start_turn("working");
|
||||
assert_eq!(thread.state, ThreadState::Processing);
|
||||
|
||||
@@ -2228,6 +2252,98 @@ mod tests {
|
||||
assert!(t.pending_messages.is_empty());
|
||||
}
|
||||
|
||||
/// Regression test for #1487: when a thread disappears from the session during
|
||||
/// approval storage, the code should return an error instead of silently losing
|
||||
/// the approval.
|
||||
#[test]
|
||||
fn test_missing_thread_during_approval_storage_returns_error() {
|
||||
use crate::agent::session::{PendingApproval, Session};
|
||||
use uuid::Uuid;
|
||||
|
||||
let thread_id = Uuid::new_v4();
|
||||
let session = Session::new("test-user");
|
||||
|
||||
// Thread does NOT exist in the session
|
||||
assert!(!session.threads.contains_key(&thread_id));
|
||||
|
||||
// Simulate the match logic from process_approval when storing a new pending approval
|
||||
let _new_pending = PendingApproval {
|
||||
request_id: Uuid::new_v4(),
|
||||
tool_name: "shell".to_string(),
|
||||
parameters: serde_json::json!({"command": "echo test"}),
|
||||
display_parameters: serde_json::json!({"command": "[REDACTED]"}),
|
||||
description: "Execute command".to_string(),
|
||||
tool_call_id: "call_0".to_string(),
|
||||
context_messages: vec![],
|
||||
deferred_tool_calls: vec![],
|
||||
user_timezone: None,
|
||||
allow_always: false,
|
||||
};
|
||||
|
||||
let tool_name = "shell";
|
||||
|
||||
// The fixed code uses match instead of if-let, returning an error for None
|
||||
let result: Result<&str, String> = match session.threads.get(&thread_id) {
|
||||
Some(_thread) => {
|
||||
// Would call thread.await_approval(new_pending)
|
||||
Ok("stored")
|
||||
}
|
||||
None => Err(format!(
|
||||
"The conversation thread was pruned during processing. Some actions may have already been executed. Tool: {}",
|
||||
tool_name,
|
||||
)),
|
||||
};
|
||||
|
||||
assert!(result.is_err(), "Missing thread should produce an error");
|
||||
let err = result.unwrap_err();
|
||||
assert!(
|
||||
err.contains("pruned during processing"),
|
||||
"Error should mention thread was pruned. Got: {}",
|
||||
err
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression test for #1487: when a thread disappears during rejection,
|
||||
/// the rejection is not persisted but the code degrades gracefully (no panic,
|
||||
/// no silent success pretending state was updated).
|
||||
#[test]
|
||||
fn test_missing_thread_during_rejection_degrades_gracefully() {
|
||||
use crate::agent::session::Session;
|
||||
use uuid::Uuid;
|
||||
|
||||
let thread_id = Uuid::new_v4();
|
||||
let mut session = Session::new("test-user");
|
||||
|
||||
// Thread does NOT exist in the session
|
||||
assert!(!session.threads.contains_key(&thread_id));
|
||||
|
||||
let rejection = format!(
|
||||
"Tool '{}' was rejected. The agent will not execute this tool.",
|
||||
"shell"
|
||||
);
|
||||
|
||||
// The fixed code uses match instead of if-let, logging a warning for None
|
||||
let mut persisted = false;
|
||||
match session.threads.get_mut(&thread_id) {
|
||||
Some(thread) => {
|
||||
thread.clear_pending_approval();
|
||||
thread.complete_turn(&rejection);
|
||||
persisted = true;
|
||||
}
|
||||
None => {
|
||||
// In production this logs a warning -- we just verify it takes
|
||||
// the None branch without panicking.
|
||||
}
|
||||
}
|
||||
|
||||
assert!(
|
||||
!persisted,
|
||||
"Rejection should NOT be persisted when thread is missing"
|
||||
);
|
||||
// Session should remain unchanged
|
||||
assert!(session.threads.is_empty());
|
||||
}
|
||||
|
||||
// Helper function to extract the approval message without needing a full Agent instance
|
||||
fn extract_approval_message(
|
||||
session: &crate::agent::session::Session,
|
||||
|
||||
-11
@@ -325,20 +325,9 @@ impl AppBuilder {
|
||||
};
|
||||
let mut ws = Workspace::new_with_db(workspace_user_id, db.clone())
|
||||
.with_search_config(&self.config.search);
|
||||
|
||||
if let Some(ref emb) = embeddings {
|
||||
ws = ws.with_embeddings_cached(emb.clone(), emb_cache_config);
|
||||
}
|
||||
|
||||
// Wire workspace-level settings (read scopes, memory layers)
|
||||
if !self.config.workspace.read_scopes.is_empty() {
|
||||
ws = ws.with_additional_read_scopes(self.config.workspace.read_scopes.clone());
|
||||
tracing::info!(
|
||||
user_id = workspace_user_id,
|
||||
read_scopes = ?ws.read_user_ids(),
|
||||
"Workspace configured with multi-scope reads"
|
||||
);
|
||||
}
|
||||
ws = ws.with_memory_layers(self.config.workspace.memory_layers.clone());
|
||||
let ws = Arc::new(ws);
|
||||
tools.register_memory_tools(Arc::clone(&ws));
|
||||
|
||||
@@ -543,7 +543,7 @@ pub async fn chat_new_thread_handler(
|
||||
let session = session_manager.get_or_create_session(&state.user_id).await;
|
||||
let (thread_id, info) = {
|
||||
let mut sess = session.lock().await;
|
||||
let thread = sess.create_thread("web");
|
||||
let thread = sess.create_thread();
|
||||
let id = thread.id;
|
||||
let info = ThreadInfo {
|
||||
id: thread.id,
|
||||
|
||||
@@ -1658,7 +1658,7 @@ async fn chat_new_thread_handler(
|
||||
let session = session_manager.get_or_create_session(&state.user_id).await;
|
||||
let (thread_id, info) = {
|
||||
let mut sess = session.lock().await;
|
||||
let thread = sess.create_thread("web");
|
||||
let thread = sess.create_thread();
|
||||
let id = thread.id;
|
||||
let info = ThreadInfo {
|
||||
id: thread.id,
|
||||
@@ -1822,13 +1822,7 @@ async fn memory_write_handler(
|
||||
"Workspace not available".to_string(),
|
||||
))?;
|
||||
|
||||
// Route through layer-aware methods when a layer is specified.
|
||||
//
|
||||
// Note: unlike MemoryWriteTool, this endpoint does NOT block writes to
|
||||
// identity files (IDENTITY.md, SOUL.md, etc.). The HTTP API is an
|
||||
// authenticated admin interface; the supervisor uses it to seed identity
|
||||
// files at startup. Identity-file protection is enforced at the tool
|
||||
// layer (LLM-facing) where the write originates from an untrusted agent.
|
||||
// Route through layer-aware methods when a layer is specified
|
||||
if let Some(ref layer_name) = req.layer {
|
||||
let result = if req.append {
|
||||
workspace
|
||||
|
||||
+7
-8
@@ -24,7 +24,7 @@ mod skills;
|
||||
mod transcription;
|
||||
mod tunnel;
|
||||
mod wasm;
|
||||
pub(crate) mod workspace;
|
||||
mod workspace;
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::sync::{LazyLock, Mutex, Once};
|
||||
@@ -178,7 +178,9 @@ impl Config {
|
||||
},
|
||||
transcription: TranscriptionConfig::default(),
|
||||
search: WorkspaceSearchConfig::default(),
|
||||
workspace: WorkspaceConfig::default(),
|
||||
workspace: WorkspaceConfig {
|
||||
memory_layers: vec![],
|
||||
},
|
||||
observability: crate::observability::ObservabilityConfig::default(),
|
||||
relay: None,
|
||||
}
|
||||
@@ -311,14 +313,11 @@ impl Config {
|
||||
|
||||
let tunnel = TunnelConfig::resolve(settings)?;
|
||||
let channels = ChannelsConfig::resolve(settings, &owner_id)?;
|
||||
|
||||
// Resolve workspace config using the gateway user_id for default layers.
|
||||
let workspace_user_id = channels
|
||||
.gateway
|
||||
.as_ref()
|
||||
.map(|gw| gw.user_id.as_str())
|
||||
.unwrap_or("default");
|
||||
let workspace = WorkspaceConfig::resolve(workspace_user_id)?;
|
||||
.map(|gw| gw.user_id.clone())
|
||||
.unwrap_or_else(|| "default".to_string());
|
||||
|
||||
Ok(Self {
|
||||
owner_id: owner_id.clone(),
|
||||
@@ -340,7 +339,7 @@ impl Config {
|
||||
skills: SkillsConfig::resolve()?,
|
||||
transcription: TranscriptionConfig::resolve(settings)?,
|
||||
search: WorkspaceSearchConfig::resolve()?,
|
||||
workspace,
|
||||
workspace: WorkspaceConfig::resolve(&workspace_user_id)?,
|
||||
observability: crate::observability::ObservabilityConfig {
|
||||
backend: std::env::var("OBSERVABILITY_BACKEND").unwrap_or_else(|_| "none".into()),
|
||||
},
|
||||
|
||||
+7
-68
@@ -2,29 +2,18 @@ use crate::config::helpers::optional_env;
|
||||
use crate::error::ConfigError;
|
||||
use crate::workspace::layer::MemoryLayer;
|
||||
|
||||
/// Workspace-level configuration (memory layers, read scopes).
|
||||
/// Workspace memory configuration.
|
||||
///
|
||||
/// Parsed from environment variables. Lives outside of `GatewayConfig`
|
||||
/// so that non-gateway channels can eventually use the same settings.
|
||||
#[derive(Debug, Clone, Default)]
|
||||
/// Controls memory layer definitions for privacy-aware writes.
|
||||
/// Layers are parsed from the `MEMORY_LAYERS` env var (JSON array)
|
||||
/// or default to a single private layer scoped to the gateway user.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct WorkspaceConfig {
|
||||
/// Memory layer definitions (JSON in `MEMORY_LAYERS` env var, or defaults).
|
||||
pub memory_layers: Vec<MemoryLayer>,
|
||||
/// Additional user scopes for workspace reads.
|
||||
///
|
||||
/// When set, the workspace can read (search, read, list) from these
|
||||
/// additional user scopes while writes remain isolated to the primary
|
||||
/// `user_id`. Parsed from `WORKSPACE_READ_SCOPES` (comma-separated).
|
||||
pub read_scopes: Vec<String>,
|
||||
}
|
||||
|
||||
impl WorkspaceConfig {
|
||||
/// Resolve workspace config from environment variables.
|
||||
///
|
||||
/// `user_id` is used to derive default memory layers when `MEMORY_LAYERS`
|
||||
/// is not set.
|
||||
pub fn resolve(user_id: &str) -> Result<Self, ConfigError> {
|
||||
// --- Memory layers ---
|
||||
pub(crate) fn resolve(user_id: &str) -> Result<Self, ConfigError> {
|
||||
let memory_layers: Vec<MemoryLayer> = match optional_env("MEMORY_LAYERS")? {
|
||||
Some(json_str) => {
|
||||
serde_json::from_str(&json_str).map_err(|e| ConfigError::InvalidValue {
|
||||
@@ -68,20 +57,6 @@ impl WorkspaceConfig {
|
||||
message: format!("layer '{}' has an empty scope", layer.name),
|
||||
});
|
||||
}
|
||||
if !layer
|
||||
.scope
|
||||
.chars()
|
||||
.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
|
||||
{
|
||||
return Err(ConfigError::InvalidValue {
|
||||
key: "MEMORY_LAYERS".to_string(),
|
||||
message: format!(
|
||||
"layer '{}' scope '{}' contains invalid characters \
|
||||
(allowed: a-z, A-Z, 0-9, _, -)",
|
||||
layer.name, layer.scope
|
||||
),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// Check for duplicate layer names
|
||||
@@ -97,43 +72,7 @@ impl WorkspaceConfig {
|
||||
}
|
||||
}
|
||||
|
||||
// --- Read scopes ---
|
||||
let read_scopes: Vec<String> = optional_env("WORKSPACE_READ_SCOPES")?
|
||||
.map(|s| {
|
||||
s.split(',')
|
||||
.map(|s| s.trim().to_string())
|
||||
.filter(|s| !s.is_empty())
|
||||
.collect()
|
||||
})
|
||||
.unwrap_or_default();
|
||||
|
||||
for scope in &read_scopes {
|
||||
if scope.len() > 128 {
|
||||
let prefix: String = scope.chars().take(32).collect();
|
||||
return Err(ConfigError::InvalidValue {
|
||||
key: "WORKSPACE_READ_SCOPES".to_string(),
|
||||
message: format!("scope '{prefix}...' exceeds 128 characters"),
|
||||
});
|
||||
}
|
||||
if !scope
|
||||
.chars()
|
||||
.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
|
||||
{
|
||||
return Err(ConfigError::InvalidValue {
|
||||
key: "WORKSPACE_READ_SCOPES".to_string(),
|
||||
message: format!(
|
||||
"scope '{}' contains invalid characters \
|
||||
(allowed: a-z, A-Z, 0-9, _, -)",
|
||||
scope
|
||||
),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
Ok(Self {
|
||||
memory_layers,
|
||||
read_scopes,
|
||||
})
|
||||
Ok(Self { memory_layers })
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -644,103 +644,6 @@ pub trait WorkspaceStore: Send + Sync {
|
||||
embedding: Option<&[f32]>,
|
||||
config: &SearchConfig,
|
||||
) -> Result<Vec<SearchResult>, WorkspaceError>;
|
||||
|
||||
// ==================== Multi-scope read methods ====================
|
||||
//
|
||||
// Default implementations loop over user_ids calling single-scope methods,
|
||||
// then merge results. Backends can override with efficient SQL (e.g.,
|
||||
// `WHERE user_id = ANY($1::text[])`).
|
||||
|
||||
/// Hybrid search across multiple user scopes, merging results by score.
|
||||
///
|
||||
/// **Note:** The default implementation calls `hybrid_search` per scope and
|
||||
/// merges by raw score. Because RRF scores are normalized independently
|
||||
/// within each scope, scores are not directly comparable across scopes.
|
||||
/// The Postgres backend overrides this with a single combined query that
|
||||
/// applies RRF once to the unified result set.
|
||||
async fn hybrid_search_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
query: &str,
|
||||
embedding: Option<&[f32]>,
|
||||
config: &SearchConfig,
|
||||
) -> Result<Vec<SearchResult>, WorkspaceError> {
|
||||
if user_ids.len() > 1 {
|
||||
tracing::debug!(
|
||||
scope_count = user_ids.len(),
|
||||
"hybrid_search_multi: using default per-scope RRF merge; \
|
||||
cross-scope score comparison may be unreliable"
|
||||
);
|
||||
}
|
||||
let mut all_results = Vec::new();
|
||||
for uid in user_ids {
|
||||
let results = self
|
||||
.hybrid_search(uid, agent_id, query, embedding, config)
|
||||
.await?;
|
||||
all_results.extend(results);
|
||||
}
|
||||
// Re-sort by score descending and truncate to limit
|
||||
all_results.sort_by(|a, b| {
|
||||
b.score
|
||||
.partial_cmp(&a.score)
|
||||
.unwrap_or(std::cmp::Ordering::Equal)
|
||||
});
|
||||
all_results.truncate(config.limit);
|
||||
Ok(all_results)
|
||||
}
|
||||
|
||||
/// List all file paths across multiple user scopes.
|
||||
async fn list_all_paths_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
) -> Result<Vec<String>, WorkspaceError> {
|
||||
let mut all_paths = Vec::new();
|
||||
for uid in user_ids {
|
||||
let paths = self.list_all_paths(uid, agent_id).await?;
|
||||
all_paths.extend(paths);
|
||||
}
|
||||
all_paths.sort();
|
||||
all_paths.dedup();
|
||||
Ok(all_paths)
|
||||
}
|
||||
|
||||
/// Get a document by path, searching across multiple user scopes.
|
||||
///
|
||||
/// Returns the first match found (tries each user_id in order).
|
||||
async fn get_document_by_path_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
path: &str,
|
||||
) -> Result<MemoryDocument, WorkspaceError> {
|
||||
for uid in user_ids {
|
||||
match self.get_document_by_path(uid, agent_id, path).await {
|
||||
Ok(doc) => return Ok(doc),
|
||||
Err(WorkspaceError::DocumentNotFound { .. }) => continue,
|
||||
Err(e) => return Err(e),
|
||||
}
|
||||
}
|
||||
Err(WorkspaceError::DocumentNotFound {
|
||||
doc_type: path.to_string(),
|
||||
user_id: format!("[{}]", user_ids.join(", ")),
|
||||
})
|
||||
}
|
||||
|
||||
/// List directory contents across multiple user scopes.
|
||||
async fn list_directory_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
directory: &str,
|
||||
) -> Result<Vec<WorkspaceEntry>, WorkspaceError> {
|
||||
let mut all_entries = Vec::new();
|
||||
for uid in user_ids {
|
||||
all_entries.extend(self.list_directory(uid, agent_id, directory).await?);
|
||||
}
|
||||
Ok(crate::workspace::merge_workspace_entries(all_entries))
|
||||
}
|
||||
}
|
||||
|
||||
/// Backend-agnostic database supertrait.
|
||||
|
||||
@@ -717,49 +717,4 @@ impl WorkspaceStore for PgBackend {
|
||||
.hybrid_search(user_id, agent_id, query, embedding, config)
|
||||
.await
|
||||
}
|
||||
|
||||
// Optimized multi-scope overrides using `ANY($1::text[])` SQL.
|
||||
|
||||
async fn hybrid_search_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
query: &str,
|
||||
embedding: Option<&[f32]>,
|
||||
config: &SearchConfig,
|
||||
) -> Result<Vec<SearchResult>, WorkspaceError> {
|
||||
self.repo
|
||||
.hybrid_search_multi(user_ids, agent_id, query, embedding, config)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn list_all_paths_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
) -> Result<Vec<String>, WorkspaceError> {
|
||||
self.repo.list_all_paths_multi(user_ids, agent_id).await
|
||||
}
|
||||
|
||||
async fn get_document_by_path_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
path: &str,
|
||||
) -> Result<MemoryDocument, WorkspaceError> {
|
||||
self.repo
|
||||
.get_document_by_path_multi(user_ids, agent_id, path)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn list_directory_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
directory: &str,
|
||||
) -> Result<Vec<WorkspaceEntry>, WorkspaceError> {
|
||||
self.repo
|
||||
.list_directory_multi(user_ids, agent_id, directory)
|
||||
.await
|
||||
}
|
||||
}
|
||||
|
||||
@@ -304,6 +304,9 @@ pub enum WorkspaceError {
|
||||
#[error("I/O error: {reason}")]
|
||||
IoError { reason: String },
|
||||
|
||||
#[error("Not found: {path}")]
|
||||
NotFound { path: String },
|
||||
|
||||
#[error("Layer not found: {name}")]
|
||||
LayerNotFound { name: String },
|
||||
|
||||
|
||||
+1
-70
@@ -28,7 +28,7 @@ use std::sync::atomic::{AtomicBool, AtomicU32, Ordering};
|
||||
|
||||
use async_trait::async_trait;
|
||||
use rust_decimal::Decimal;
|
||||
use tokio::sync::{Mutex as AsyncMutex, mpsc};
|
||||
use tokio::sync::mpsc;
|
||||
|
||||
use crate::agent::AgentDeps;
|
||||
use crate::channels::{
|
||||
@@ -361,75 +361,6 @@ impl Channel for StubChannel {
|
||||
}
|
||||
}
|
||||
|
||||
/// Captured broadcast deliveries keyed by the target user or chat identifier.
|
||||
pub type BroadcastCapture = Arc<AsyncMutex<Vec<(String, OutgoingResponse)>>>;
|
||||
|
||||
/// A lightweight channel double that only records `broadcast()` traffic.
|
||||
///
|
||||
/// This is useful for unit tests that need to assert message routing without
|
||||
/// spinning up a full interactive channel harness.
|
||||
pub struct RecordingBroadcastChannel {
|
||||
name: &'static str,
|
||||
captures: BroadcastCapture,
|
||||
}
|
||||
|
||||
impl RecordingBroadcastChannel {
|
||||
pub fn new(name: &'static str) -> (Self, BroadcastCapture) {
|
||||
let captures = Arc::new(AsyncMutex::new(Vec::new()));
|
||||
(
|
||||
Self {
|
||||
name,
|
||||
captures: Arc::clone(&captures),
|
||||
},
|
||||
captures,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl Channel for RecordingBroadcastChannel {
|
||||
fn name(&self) -> &str {
|
||||
self.name
|
||||
}
|
||||
|
||||
async fn start(&self) -> Result<MessageStream, ChannelError> {
|
||||
let (_tx, rx) = mpsc::channel::<IncomingMessage>(1);
|
||||
Ok(Box::pin(tokio_stream::wrappers::ReceiverStream::new(rx)))
|
||||
}
|
||||
|
||||
async fn respond(
|
||||
&self,
|
||||
_msg: &IncomingMessage,
|
||||
_response: OutgoingResponse,
|
||||
) -> Result<(), ChannelError> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn send_status(
|
||||
&self,
|
||||
_status: StatusUpdate,
|
||||
_metadata: &serde_json::Value,
|
||||
) -> Result<(), ChannelError> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn broadcast(
|
||||
&self,
|
||||
user_id: &str,
|
||||
response: OutgoingResponse,
|
||||
) -> Result<(), ChannelError> {
|
||||
self.captures
|
||||
.lock()
|
||||
.await
|
||||
.push((user_id.to_string(), response));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn health_check(&self) -> Result<(), ChannelError> {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
/// Assembled test components.
|
||||
pub struct TestHarness {
|
||||
/// The agent dependencies, ready for use.
|
||||
|
||||
@@ -271,13 +271,12 @@ impl Tool for MemoryWriteTool {
|
||||
.and_then(|v| v.as_bool())
|
||||
.unwrap_or(false);
|
||||
|
||||
// Parse timezone once for targets that need it (daily_log).
|
||||
let tz = crate::timezone::parse_timezone(&ctx.user_timezone).unwrap_or(chrono_tz::Tz::UTC);
|
||||
|
||||
// Resolve the target to a workspace path
|
||||
let resolved_path = match target {
|
||||
"memory" => paths::MEMORY.to_string(),
|
||||
"daily_log" => {
|
||||
let tz = crate::timezone::parse_timezone(&ctx.user_timezone)
|
||||
.unwrap_or(chrono_tz::Tz::UTC);
|
||||
let now = chrono::Utc::now().with_timezone(&tz);
|
||||
format!("daily/{}.md", now.format("%Y-%m-%d"))
|
||||
}
|
||||
@@ -319,6 +318,8 @@ impl Tool for MemoryWriteTool {
|
||||
}
|
||||
}
|
||||
"daily_log" => {
|
||||
let tz = crate::timezone::parse_timezone(&ctx.user_timezone)
|
||||
.unwrap_or(chrono_tz::Tz::UTC);
|
||||
self.workspace
|
||||
.append_daily_log_tz(content, tz)
|
||||
.await
|
||||
|
||||
@@ -80,12 +80,6 @@ fn metadata_notify_user(metadata: &serde_json::Value) -> Option<String> {
|
||||
metadata_string(metadata, "notify_user").filter(|value| value != "default")
|
||||
}
|
||||
|
||||
// Autonomous runs include `owner_id` when the job is executing on behalf of a
|
||||
// durable owner scope instead of an interactive channel actor.
|
||||
fn metadata_owner_id(metadata: &serde_json::Value) -> Option<String> {
|
||||
metadata_string(metadata, "owner_id")
|
||||
}
|
||||
|
||||
fn channel_matches_source(resolved_channel: Option<&str>, source_channel: Option<&str>) -> bool {
|
||||
match (resolved_channel, source_channel) {
|
||||
(None, _) => true,
|
||||
@@ -97,13 +91,11 @@ fn channel_matches_source(resolved_channel: Option<&str>, source_channel: Option
|
||||
async fn resolve_channel_fallback_target(
|
||||
extension_manager: Option<&Arc<ExtensionManager>>,
|
||||
channel: Option<&str>,
|
||||
owner_scope_target: Option<&str>,
|
||||
ctx_user_id: &str,
|
||||
) -> Option<String> {
|
||||
// Prefer an explicit channel binding when the extension manager knows the
|
||||
// durable delivery target (for example, a bound Telegram chat ID).
|
||||
if let Some(channel_name) = channel
|
||||
&& let Some(extension_manager) = extension_manager
|
||||
let channel_name = channel?;
|
||||
|
||||
if let Some(extension_manager) = extension_manager
|
||||
&& let Some(target) = extension_manager
|
||||
.notification_target_for_channel(channel_name)
|
||||
.await
|
||||
@@ -111,19 +103,13 @@ async fn resolve_channel_fallback_target(
|
||||
return Some(target);
|
||||
}
|
||||
|
||||
// `owner_id` is only present for autonomous owner-scoped executions.
|
||||
// Interactive chat turns intentionally fall back to `ctx.user_id`, which is
|
||||
// already the active conversation target for the current channel.
|
||||
owner_scope_target
|
||||
.map(ToOwned::to_owned)
|
||||
.or_else(|| Some(ctx_user_id.to_string()))
|
||||
Some(ctx_user_id.to_string())
|
||||
}
|
||||
|
||||
struct MessageTargetResolution<'a> {
|
||||
extension_manager: Option<&'a Arc<ExtensionManager>>,
|
||||
explicit_target: Option<String>,
|
||||
metadata_target: Option<String>,
|
||||
owner_scope_target: Option<String>,
|
||||
default_target: Option<String>,
|
||||
channel: Option<&'a str>,
|
||||
metadata_channel: Option<&'a str>,
|
||||
@@ -147,7 +133,6 @@ async fn resolve_message_target(inputs: MessageTargetResolution<'_>) -> Option<S
|
||||
return resolve_channel_fallback_target(
|
||||
inputs.extension_manager,
|
||||
inputs.channel,
|
||||
inputs.owner_scope_target.as_deref(),
|
||||
inputs.ctx_user_id,
|
||||
)
|
||||
.await;
|
||||
@@ -160,12 +145,9 @@ async fn resolve_message_target(inputs: MessageTargetResolution<'_>) -> Option<S
|
||||
}
|
||||
|
||||
if inputs.channel.is_some() {
|
||||
// Shared per-turn conversation defaults are already scoped to the
|
||||
// active interactive target, so owner scope metadata is irrelevant.
|
||||
return resolve_channel_fallback_target(
|
||||
inputs.extension_manager,
|
||||
inputs.channel,
|
||||
None,
|
||||
inputs.ctx_user_id,
|
||||
)
|
||||
.await;
|
||||
@@ -242,9 +224,8 @@ impl Tool for MessageTool {
|
||||
.unwrap_or_else(|e| e.into_inner())
|
||||
.clone();
|
||||
let metadata_target = metadata_notify_user(&ctx.metadata);
|
||||
let owner_scope_target = metadata_owner_id(&ctx.metadata);
|
||||
let has_execution_routing_metadata =
|
||||
metadata_channel.is_some() || metadata_target.is_some() || owner_scope_target.is_some();
|
||||
metadata_channel.is_some() || metadata_target.is_some();
|
||||
|
||||
// Job metadata is authoritative for autonomous executions. The shared
|
||||
// conversation defaults are only a legacy fallback when no execution-local
|
||||
@@ -269,7 +250,6 @@ impl Tool for MessageTool {
|
||||
extension_manager: self.extension_manager.as_ref(),
|
||||
explicit_target,
|
||||
metadata_target,
|
||||
owner_scope_target,
|
||||
default_target,
|
||||
channel: channel.as_deref(),
|
||||
metadata_channel: metadata_channel.as_deref(),
|
||||
@@ -425,13 +405,83 @@ impl Tool for MessageTool {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::testing::{BroadcastCapture, RecordingBroadcastChannel};
|
||||
use async_trait::async_trait;
|
||||
use tokio::sync::{Mutex, mpsc};
|
||||
|
||||
use crate::channels::{
|
||||
Channel, IncomingMessage, MessageStream, OutgoingResponse, StatusUpdate,
|
||||
};
|
||||
use crate::error::ChannelError;
|
||||
|
||||
type BroadcastCapture = Arc<Mutex<Vec<(String, OutgoingResponse)>>>;
|
||||
|
||||
struct RecordingChannel {
|
||||
name: &'static str,
|
||||
captures: BroadcastCapture,
|
||||
}
|
||||
|
||||
impl RecordingChannel {
|
||||
fn new(name: &'static str) -> (Self, BroadcastCapture) {
|
||||
let captures = Arc::new(Mutex::new(Vec::new()));
|
||||
(
|
||||
Self {
|
||||
name,
|
||||
captures: Arc::clone(&captures),
|
||||
},
|
||||
captures,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl Channel for RecordingChannel {
|
||||
fn name(&self) -> &str {
|
||||
self.name
|
||||
}
|
||||
|
||||
async fn start(&self) -> Result<MessageStream, ChannelError> {
|
||||
let (_tx, rx) = mpsc::channel::<IncomingMessage>(1);
|
||||
Ok(Box::pin(tokio_stream::wrappers::ReceiverStream::new(rx)))
|
||||
}
|
||||
|
||||
async fn respond(
|
||||
&self,
|
||||
_msg: &IncomingMessage,
|
||||
_response: OutgoingResponse,
|
||||
) -> Result<(), ChannelError> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn send_status(
|
||||
&self,
|
||||
_status: StatusUpdate,
|
||||
_metadata: &serde_json::Value,
|
||||
) -> Result<(), ChannelError> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn broadcast(
|
||||
&self,
|
||||
user_id: &str,
|
||||
response: OutgoingResponse,
|
||||
) -> Result<(), ChannelError> {
|
||||
self.captures
|
||||
.lock()
|
||||
.await
|
||||
.push((user_id.to_string(), response));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn health_check(&self) -> Result<(), ChannelError> {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
async fn message_tool_with_recording_channels()
|
||||
-> (MessageTool, BroadcastCapture, BroadcastCapture) {
|
||||
let channel_manager = ChannelManager::new();
|
||||
let (gateway, gateway_captures) = RecordingBroadcastChannel::new("gateway");
|
||||
let (telegram, telegram_captures) = RecordingBroadcastChannel::new("telegram");
|
||||
let (gateway, gateway_captures) = RecordingChannel::new("gateway");
|
||||
let (telegram, telegram_captures) = RecordingChannel::new("telegram");
|
||||
channel_manager.add(Box::new(gateway)).await;
|
||||
channel_manager.add(Box::new(telegram)).await;
|
||||
|
||||
@@ -820,63 +870,28 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn message_tool_falls_back_to_owner_scope_when_channel_known() {
|
||||
let (tool, gateway_captures, telegram_captures) =
|
||||
message_tool_with_recording_channels().await;
|
||||
async fn message_tool_falls_back_to_ctx_user_when_channel_known() {
|
||||
// Regression for owner-scoped notifications: a channel can be known
|
||||
// even when the concrete delivery target is omitted, so the message
|
||||
// tool should pass ctx.user_id through to the channel layer.
|
||||
let tool = MessageTool::new(Arc::new(ChannelManager::new()));
|
||||
|
||||
let mut ctx =
|
||||
crate::context::JobContext::with_user("telegram", "routine-job", "price alert");
|
||||
ctx.metadata = serde_json::json!({
|
||||
"notify_channel": "telegram",
|
||||
"owner_id": "owner-scope",
|
||||
});
|
||||
|
||||
let result = tool
|
||||
.execute(serde_json::json!({"content": "NEAR price is $5"}), &ctx)
|
||||
.await
|
||||
.expect("message tool should use owner scope before ctx.user_id");
|
||||
|
||||
assert_eq!(
|
||||
result.result.as_str(),
|
||||
Some("Sent message to telegram:owner-scope")
|
||||
);
|
||||
assert!(gateway_captures.lock().await.is_empty());
|
||||
let telegram = telegram_captures.lock().await.clone();
|
||||
assert_eq!(telegram.len(), 1);
|
||||
assert_eq!(telegram[0].0, "owner-scope");
|
||||
assert_eq!(telegram[0].1.content, "NEAR price is $5");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn message_tool_falls_back_to_ctx_user_when_owner_scope_absent() {
|
||||
let (tool, gateway_captures, telegram_captures) =
|
||||
message_tool_with_recording_channels().await;
|
||||
|
||||
let mut ctx = crate::context::JobContext::with_user(
|
||||
"interactive-chat-user",
|
||||
"routine-job",
|
||||
"price alert",
|
||||
);
|
||||
crate::context::JobContext::with_user("owner-scope", "routine-job", "price alert");
|
||||
ctx.metadata = serde_json::json!({
|
||||
"notify_channel": "telegram",
|
||||
});
|
||||
|
||||
let result = tool
|
||||
.execute(serde_json::json!({"content": "NEAR price is $5"}), &ctx)
|
||||
.await
|
||||
.expect(
|
||||
"message tool should fall back to ctx.user_id when owner scope metadata is absent",
|
||||
);
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
result.result.as_str(),
|
||||
Some("Sent message to telegram:interactive-chat-user")
|
||||
);
|
||||
assert!(gateway_captures.lock().await.is_empty());
|
||||
let telegram = telegram_captures.lock().await.clone();
|
||||
assert_eq!(telegram.len(), 1);
|
||||
assert_eq!(telegram[0].0, "interactive-chat-user");
|
||||
assert_eq!(telegram[0].1.content, "NEAR price is $5");
|
||||
assert!(result.is_err()); // safety: test-only assertion
|
||||
let err = result.unwrap_err().to_string();
|
||||
let mentions_missing_target = err.contains("No target specified");
|
||||
assert!(!mentions_missing_target); // safety: test-only assertion
|
||||
let mentions_missing_channel = err.contains("No channel specified");
|
||||
assert!(!mentions_missing_channel); // safety: test-only assertion
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -1438,9 +1438,6 @@ impl From<TaskOutput> for Result<String, Error> {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use std::sync::Arc;
|
||||
|
||||
use crate::channels::ChannelManager;
|
||||
use crate::llm::ToolSelection;
|
||||
|
||||
use super::*;
|
||||
@@ -1451,8 +1448,6 @@ mod tests {
|
||||
ToolCompletionResponse,
|
||||
};
|
||||
use crate::safety::SafetyLayer;
|
||||
use crate::testing::{BroadcastCapture, RecordingBroadcastChannel};
|
||||
use crate::tools::builtin::MessageTool;
|
||||
use crate::tools::{Tool, ToolError as ToolExecError, ToolOutput};
|
||||
|
||||
/// A test tool that sleeps for a configurable duration before returning.
|
||||
@@ -1544,20 +1539,6 @@ mod tests {
|
||||
Worker::new(job_id, deps)
|
||||
}
|
||||
|
||||
async fn make_worker_with_message_tool()
|
||||
-> (Worker, Arc<MessageTool>, BroadcastCapture, BroadcastCapture) {
|
||||
let channel_manager = ChannelManager::new();
|
||||
let (gateway, gateway_captures) = RecordingBroadcastChannel::new("gateway");
|
||||
let (telegram, telegram_captures) = RecordingBroadcastChannel::new("telegram");
|
||||
channel_manager.add(Box::new(gateway)).await;
|
||||
channel_manager.add(Box::new(telegram)).await;
|
||||
|
||||
let message_tool = Arc::new(MessageTool::new(Arc::new(channel_manager)));
|
||||
let worker = make_worker(vec![message_tool.clone()]).await;
|
||||
|
||||
(worker, message_tool, gateway_captures, telegram_captures)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tool_selection_preserves_call_id() {
|
||||
let selection = ToolSelection {
|
||||
@@ -2166,50 +2147,4 @@ mod tests {
|
||||
|
||||
assert_eq!(ctx.metadata, original); // safety: test
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn autonomous_message_tool_ignores_stale_gateway_context_when_routine_metadata_targets_telegram()
|
||||
{
|
||||
let (worker, message_tool, gateway_captures, telegram_captures) =
|
||||
make_worker_with_message_tool().await;
|
||||
|
||||
message_tool
|
||||
.set_context(
|
||||
Some("gateway".to_string()),
|
||||
Some("stale-gateway-target".to_string()),
|
||||
)
|
||||
.await;
|
||||
|
||||
worker
|
||||
.context_manager()
|
||||
.update_context(worker.job_id, |ctx| {
|
||||
ctx.user_id = "telegram".to_string();
|
||||
ctx.metadata = serde_json::json!({
|
||||
"notify_channel": "telegram",
|
||||
"owner_id": "owner-scope",
|
||||
});
|
||||
Ok::<(), String>(())
|
||||
})
|
||||
.await
|
||||
.unwrap() // safety: test
|
||||
.unwrap(); // safety: test
|
||||
|
||||
let result = worker
|
||||
.execute_tool(
|
||||
"message",
|
||||
&serde_json::json!({"content": "hello from routine"}),
|
||||
)
|
||||
.await
|
||||
.unwrap(); // safety: test
|
||||
assert!(
|
||||
result.contains("telegram:owner-scope"),
|
||||
"expected telegram owner-scope routing, got: {result}"
|
||||
);
|
||||
|
||||
assert!(gateway_captures.lock().await.is_empty());
|
||||
let telegram = telegram_captures.lock().await.clone();
|
||||
assert_eq!(telegram.len(), 1);
|
||||
assert_eq!(telegram[0].0, "owner-scope");
|
||||
assert_eq!(telegram[0].1.content, "hello from routine");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -91,27 +91,6 @@ Default k=60. Results from both methods are combined, with documents appearing i
|
||||
- **PostgreSQL:** `ts_rank_cd` for FTS, pgvector cosine distance for vectors, full RRF
|
||||
- **libSQL:** FTS5 for keyword search + vector search via `libsql_vector_idx` (dimension set dynamically by `ensure_vector_index()` during startup)
|
||||
|
||||
## Multi-Scope Reads & Identity Isolation
|
||||
|
||||
When a workspace has additional read scopes (via `with_additional_read_scopes`), read operations can span multiple user scopes — a user with scopes `["alice", "shared"]` can read documents from both.
|
||||
|
||||
**Identity files are exempt from multi-scope reads.** The system prompt reads identity and configuration files from the **primary scope only** (`read_primary()`), never from secondary scopes:
|
||||
|
||||
| File | Read method | Rationale |
|
||||
|------|------------|-----------|
|
||||
| AGENTS.md | `read_primary()` | Agent instructions are per-user |
|
||||
| SOUL.md | `read_primary()` | Core values are per-user |
|
||||
| USER.md | `read_primary()` | User context is per-user |
|
||||
| IDENTITY.md | `read_primary()` | Identity is per-user |
|
||||
| TOOLS.md | `read_primary()` | Tool config is per-user |
|
||||
| BOOTSTRAP.md | `read_primary()` | Onboarding is per-user |
|
||||
| MEMORY.md | `read()` | Shared memory is a feature |
|
||||
| daily/*.md | `read()` | Shared daily logs are a feature |
|
||||
|
||||
**Why:** Without this, a user with read access to another scope could silently inherit that scope's identity if their own copy is missing. The agent would present itself as the wrong user — a correctness and security issue.
|
||||
|
||||
**Design rule:** If you want shared identity across users, seed the same content into each user's scope at setup time. Don't rely on multi-scope fallback for identity files.
|
||||
|
||||
## Heartbeat System
|
||||
|
||||
Proactive periodic execution (default: 30 minutes):
|
||||
|
||||
+4
-167
@@ -37,25 +37,6 @@ pub mod paths {
|
||||
pub const ASSISTANT_DIRECTIVES: &str = "context/assistant-directives.md";
|
||||
}
|
||||
|
||||
/// Paths treated as identity documents for multi-scope isolation.
|
||||
///
|
||||
/// These files are always read from the primary scope only — never from
|
||||
/// secondary read scopes. This prevents silent identity inheritance
|
||||
/// (e.g., user A accidentally presenting as user B).
|
||||
pub const IDENTITY_PATHS: &[&str] = &[
|
||||
paths::IDENTITY,
|
||||
paths::SOUL,
|
||||
paths::AGENTS,
|
||||
paths::USER,
|
||||
paths::TOOLS,
|
||||
paths::BOOTSTRAP,
|
||||
];
|
||||
|
||||
/// Check if a path is an identity document that must be isolated to primary scope.
|
||||
pub fn is_identity_path(path: &str) -> bool {
|
||||
IDENTITY_PATHS.contains(&path)
|
||||
}
|
||||
|
||||
/// A memory document stored in the database.
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
pub struct MemoryDocument {
|
||||
@@ -120,7 +101,10 @@ impl MemoryDocument {
|
||||
|
||||
/// Check if this is a well-known identity document.
|
||||
pub fn is_identity_document(&self) -> bool {
|
||||
is_identity_path(&self.path)
|
||||
matches!(
|
||||
self.path.as_str(),
|
||||
paths::IDENTITY | paths::SOUL | paths::AGENTS | paths::USER
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -144,42 +128,6 @@ impl WorkspaceEntry {
|
||||
}
|
||||
}
|
||||
|
||||
/// Merge workspace entries from multiple scopes into a deduplicated, sorted list.
|
||||
///
|
||||
/// When the same path appears in multiple scopes:
|
||||
/// - Keeps the most recent `updated_at`
|
||||
/// - If any scope marks it as a directory, the merged entry is a directory
|
||||
pub fn merge_workspace_entries(
|
||||
entries: impl IntoIterator<Item = WorkspaceEntry>,
|
||||
) -> Vec<WorkspaceEntry> {
|
||||
let mut seen = std::collections::HashMap::new();
|
||||
for entry in entries {
|
||||
seen.entry(entry.path.clone())
|
||||
.and_modify(|existing: &mut WorkspaceEntry| {
|
||||
// Keep the most recent updated_at (and its content_preview)
|
||||
if let (Some(existing_ts), Some(new_ts)) = (&existing.updated_at, &entry.updated_at)
|
||||
{
|
||||
if new_ts > existing_ts {
|
||||
existing.updated_at = Some(*new_ts);
|
||||
existing.content_preview = entry.content_preview.clone();
|
||||
}
|
||||
} else if existing.updated_at.is_none() {
|
||||
existing.updated_at = entry.updated_at;
|
||||
existing.content_preview = entry.content_preview.clone();
|
||||
}
|
||||
// If either is a directory, mark as directory
|
||||
if entry.is_directory {
|
||||
existing.is_directory = true;
|
||||
existing.content_preview = None;
|
||||
}
|
||||
})
|
||||
.or_insert(entry);
|
||||
}
|
||||
let mut result: Vec<WorkspaceEntry> = seen.into_values().collect();
|
||||
result.sort_by(|a, b| a.path.cmp(&b.path));
|
||||
result
|
||||
}
|
||||
|
||||
/// A chunk of a memory document for search indexing.
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
pub struct MemoryChunk {
|
||||
@@ -278,115 +226,4 @@ mod tests {
|
||||
};
|
||||
assert_eq!(entry.name(), "alpha");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_merge_workspace_entries_empty() {
|
||||
let result = merge_workspace_entries(vec![]);
|
||||
assert!(result.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_merge_workspace_entries_keeps_newer_timestamp_and_preview() {
|
||||
use chrono::TimeZone;
|
||||
let old_ts = chrono::Utc.with_ymd_and_hms(2025, 1, 1, 0, 0, 0).unwrap();
|
||||
let new_ts = chrono::Utc.with_ymd_and_hms(2025, 6, 1, 0, 0, 0).unwrap();
|
||||
|
||||
let entries = vec![
|
||||
WorkspaceEntry {
|
||||
path: "notes.md".to_string(),
|
||||
is_directory: false,
|
||||
updated_at: Some(old_ts),
|
||||
content_preview: Some("old".to_string()),
|
||||
},
|
||||
WorkspaceEntry {
|
||||
path: "notes.md".to_string(),
|
||||
is_directory: false,
|
||||
updated_at: Some(new_ts),
|
||||
content_preview: Some("new".to_string()),
|
||||
},
|
||||
];
|
||||
|
||||
let result = merge_workspace_entries(entries);
|
||||
assert_eq!(result.len(), 1);
|
||||
assert_eq!(result[0].updated_at, Some(new_ts));
|
||||
assert_eq!(result[0].content_preview, Some("new".to_string()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_merge_workspace_entries_directory_wins() {
|
||||
let entries = vec![
|
||||
WorkspaceEntry {
|
||||
path: "projects".to_string(),
|
||||
is_directory: false,
|
||||
updated_at: None,
|
||||
content_preview: Some("file content".to_string()),
|
||||
},
|
||||
WorkspaceEntry {
|
||||
path: "projects".to_string(),
|
||||
is_directory: true,
|
||||
updated_at: None,
|
||||
content_preview: None,
|
||||
},
|
||||
];
|
||||
|
||||
let result = merge_workspace_entries(entries);
|
||||
assert_eq!(result.len(), 1);
|
||||
assert!(result[0].is_directory);
|
||||
assert!(result[0].content_preview.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_merge_workspace_entries_fills_missing_timestamp() {
|
||||
use chrono::TimeZone;
|
||||
let ts = chrono::Utc.with_ymd_and_hms(2025, 3, 1, 0, 0, 0).unwrap();
|
||||
|
||||
let entries = vec![
|
||||
WorkspaceEntry {
|
||||
path: "a.md".to_string(),
|
||||
is_directory: false,
|
||||
updated_at: None,
|
||||
content_preview: None,
|
||||
},
|
||||
WorkspaceEntry {
|
||||
path: "a.md".to_string(),
|
||||
is_directory: false,
|
||||
updated_at: Some(ts),
|
||||
content_preview: None,
|
||||
},
|
||||
];
|
||||
|
||||
let result = merge_workspace_entries(entries);
|
||||
assert_eq!(result.len(), 1);
|
||||
assert_eq!(result[0].updated_at, Some(ts));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_merge_workspace_entries_sorted_by_path() {
|
||||
let entries = vec![
|
||||
WorkspaceEntry {
|
||||
path: "z.md".to_string(),
|
||||
is_directory: false,
|
||||
updated_at: None,
|
||||
content_preview: None,
|
||||
},
|
||||
WorkspaceEntry {
|
||||
path: "a.md".to_string(),
|
||||
is_directory: false,
|
||||
updated_at: None,
|
||||
content_preview: None,
|
||||
},
|
||||
WorkspaceEntry {
|
||||
path: "m.md".to_string(),
|
||||
is_directory: false,
|
||||
updated_at: None,
|
||||
content_preview: None,
|
||||
},
|
||||
];
|
||||
|
||||
let result = merge_workspace_entries(entries);
|
||||
assert_eq!(result.len(), 3);
|
||||
assert_eq!(result[0].path, "a.md");
|
||||
assert_eq!(result[1].path, "m.md");
|
||||
assert_eq!(result[2].path, "z.md");
|
||||
}
|
||||
}
|
||||
|
||||
+38
-362
@@ -52,10 +52,7 @@ mod repository;
|
||||
mod search;
|
||||
|
||||
pub use chunker::{ChunkConfig, chunk_document};
|
||||
pub use document::{
|
||||
IDENTITY_PATHS, MemoryChunk, MemoryDocument, WorkspaceEntry, is_identity_path,
|
||||
merge_workspace_entries, paths,
|
||||
};
|
||||
pub use document::{MemoryChunk, MemoryDocument, WorkspaceEntry, paths};
|
||||
pub use embedding_cache::{CachedEmbeddingProvider, EmbeddingCacheConfig};
|
||||
pub use embeddings::{
|
||||
EmbeddingProvider, MockEmbeddings, NearAiEmbeddings, OllamaEmbeddings, OpenAiEmbeddings,
|
||||
@@ -323,48 +320,6 @@ impl WorkspaceStorage {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ==================== Multi-scope read methods ====================
|
||||
|
||||
async fn hybrid_search_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
query: &str,
|
||||
embedding: Option<&[f32]>,
|
||||
config: &SearchConfig,
|
||||
) -> Result<Vec<SearchResult>, WorkspaceError> {
|
||||
match self {
|
||||
#[cfg(feature = "postgres")]
|
||||
Self::Repo(repo) => {
|
||||
repo.hybrid_search_multi(user_ids, agent_id, query, embedding, config)
|
||||
.await
|
||||
}
|
||||
Self::Db(db) => {
|
||||
db.hybrid_search_multi(user_ids, agent_id, query, embedding, config)
|
||||
.await
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn get_document_by_path_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
path: &str,
|
||||
) -> Result<MemoryDocument, WorkspaceError> {
|
||||
match self {
|
||||
#[cfg(feature = "postgres")]
|
||||
Self::Repo(repo) => {
|
||||
repo.get_document_by_path_multi(user_ids, agent_id, path)
|
||||
.await
|
||||
}
|
||||
Self::Db(db) => {
|
||||
db.get_document_by_path_multi(user_ids, agent_id, path)
|
||||
.await
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Default template seeded into HEARTBEAT.md on first access.
|
||||
@@ -385,20 +340,9 @@ const BOOTSTRAP_SEED: &str = include_str!("seeds/BOOTSTRAP.md");
|
||||
/// Each workspace is scoped to a user (and optionally an agent).
|
||||
/// Documents are persisted to the database and indexed for search.
|
||||
/// Supports both PostgreSQL (via Repository) and libSQL (via Database trait).
|
||||
///
|
||||
/// ## Multi-scope reads
|
||||
///
|
||||
/// By default, a workspace reads from and writes to a single `user_id`.
|
||||
/// With `with_additional_read_scopes`, read operations (search, read, list)
|
||||
/// can span multiple user scopes while writes remain isolated to the primary
|
||||
/// `user_id`. This enables cross-tenant read access (e.g., a user reading
|
||||
/// from both their own workspace and a "shared" workspace).
|
||||
pub struct Workspace {
|
||||
/// User identifier (from channel). All writes go to this scope.
|
||||
/// User identifier (from channel).
|
||||
user_id: String,
|
||||
/// User identifiers for read operations. Includes `user_id` as the first
|
||||
/// element, plus any additional scopes added via `with_additional_read_scopes`.
|
||||
read_user_ids: Vec<String>,
|
||||
/// Optional agent ID for multi-agent isolation.
|
||||
agent_id: Option<Uuid>,
|
||||
/// Database storage backend.
|
||||
@@ -427,7 +371,6 @@ impl Workspace {
|
||||
let user_id_str = user_id.into();
|
||||
let memory_layers = crate::workspace::layer::MemoryLayer::default_for_user(&user_id_str);
|
||||
Self {
|
||||
read_user_ids: vec![user_id_str.clone()],
|
||||
user_id: user_id_str,
|
||||
agent_id: None,
|
||||
storage: WorkspaceStorage::Repo(Repository::new(pool)),
|
||||
@@ -447,7 +390,6 @@ impl Workspace {
|
||||
let user_id_str = user_id.into();
|
||||
let memory_layers = crate::workspace::layer::MemoryLayer::default_for_user(&user_id_str);
|
||||
Self {
|
||||
read_user_ids: vec![user_id_str.clone()],
|
||||
user_id: user_id_str,
|
||||
agent_id: None,
|
||||
storage: WorkspaceStorage::Db(db),
|
||||
@@ -532,12 +474,6 @@ impl Workspace {
|
||||
///
|
||||
/// Also updates read_user_ids to include all layer scopes.
|
||||
pub fn with_memory_layers(mut self, layers: Vec<crate::workspace::layer::MemoryLayer>) -> Self {
|
||||
// Add layer scopes to read_user_ids (same dedup logic as with_additional_read_scopes)
|
||||
for layer in &layers {
|
||||
if !self.read_user_ids.contains(&layer.scope) {
|
||||
self.read_user_ids.push(layer.scope.clone());
|
||||
}
|
||||
}
|
||||
self.memory_layers = layers;
|
||||
self
|
||||
}
|
||||
@@ -560,37 +496,11 @@ impl Workspace {
|
||||
&self.memory_layers
|
||||
}
|
||||
|
||||
/// Add additional user scopes for read operations.
|
||||
///
|
||||
/// The primary `user_id` is always included. Additional scopes allow
|
||||
/// read operations (search, read, list) to span multiple tenants while
|
||||
/// writes remain isolated to the primary scope.
|
||||
///
|
||||
/// Duplicate scopes are ignored.
|
||||
pub fn with_additional_read_scopes(mut self, scopes: Vec<String>) -> Self {
|
||||
for scope in scopes {
|
||||
if !self.read_user_ids.contains(&scope) {
|
||||
self.read_user_ids.push(scope);
|
||||
}
|
||||
}
|
||||
self
|
||||
}
|
||||
|
||||
/// Get the user ID (primary scope for writes).
|
||||
/// Get the user ID.
|
||||
pub fn user_id(&self) -> &str {
|
||||
&self.user_id
|
||||
}
|
||||
|
||||
/// Get the user IDs used for read operations.
|
||||
pub fn read_user_ids(&self) -> &[String] {
|
||||
&self.read_user_ids
|
||||
}
|
||||
|
||||
/// Whether this workspace has multiple read scopes.
|
||||
fn is_multi_scope(&self) -> bool {
|
||||
self.read_user_ids.len() > 1
|
||||
}
|
||||
|
||||
/// Get the agent ID.
|
||||
pub fn agent_id(&self) -> Option<Uuid> {
|
||||
self.agent_id
|
||||
@@ -608,33 +518,6 @@ impl Workspace {
|
||||
/// println!("{}", doc.content);
|
||||
/// ```
|
||||
pub async fn read(&self, path: &str) -> Result<MemoryDocument, WorkspaceError> {
|
||||
let path = normalize_path(path);
|
||||
if self.is_multi_scope() && is_identity_path(&path) {
|
||||
// Identity files must only come from the primary scope.
|
||||
self.storage
|
||||
.get_document_by_path(&self.user_id, self.agent_id, &path)
|
||||
.await
|
||||
} else if self.is_multi_scope() {
|
||||
self.storage
|
||||
.get_document_by_path_multi(&self.read_user_ids, self.agent_id, &path)
|
||||
.await
|
||||
} else {
|
||||
self.storage
|
||||
.get_document_by_path(&self.user_id, self.agent_id, &path)
|
||||
.await
|
||||
}
|
||||
}
|
||||
|
||||
/// Read a file from the **primary scope only**, ignoring additional read scopes.
|
||||
///
|
||||
/// Use this for identity and configuration files (AGENTS.md, SOUL.md, USER.md,
|
||||
/// IDENTITY.md, TOOLS.md, BOOTSTRAP.md) where inheriting content from another
|
||||
/// scope would be a correctness/security issue — the agent must never silently
|
||||
/// present itself as the wrong user.
|
||||
///
|
||||
/// For memory files that should span scopes (MEMORY.md, daily logs), use
|
||||
/// [`read`] instead.
|
||||
pub async fn read_primary(&self, path: &str) -> Result<MemoryDocument, WorkspaceError> {
|
||||
let path = normalize_path(path);
|
||||
self.storage
|
||||
.get_document_by_path(&self.user_id, self.agent_id, &path)
|
||||
@@ -673,9 +556,6 @@ impl Workspace {
|
||||
/// Uses a single `\n` separator (suitable for log-style entries).
|
||||
/// For semantic separation (e.g., memory entries), use `append_memory()`
|
||||
/// which uses `\n\n`.
|
||||
///
|
||||
/// Uses a read-modify-write pattern that is not concurrency-safe:
|
||||
/// concurrent appends to the same path may lose writes.
|
||||
pub async fn append(&self, path: &str, content: &str) -> Result<(), WorkspaceError> {
|
||||
let path = normalize_path(path);
|
||||
// Scan system-prompt-injected files for prompt injection.
|
||||
@@ -796,20 +676,6 @@ impl Workspace {
|
||||
}
|
||||
|
||||
/// Write to a layer, with append semantics.
|
||||
///
|
||||
/// Note: privacy classification only examines the new `content`, not the
|
||||
/// full document after concatenation. See [`PatternPrivacyClassifier`]
|
||||
/// limitations for details.
|
||||
///
|
||||
/// When a privacy redirect occurs, the append targets a **separate
|
||||
/// document** in the private scope at the same path — the shared-scope
|
||||
/// document is left unmodified. Subsequent multi-scope reads will return
|
||||
/// the private copy (primary scope wins), effectively shadowing the
|
||||
/// shared document at that path. The `WriteResult::redirected` flag
|
||||
/// indicates when this has happened.
|
||||
///
|
||||
/// Uses a read-modify-write pattern that is not concurrency-safe:
|
||||
/// concurrent appends to the same path may lose writes.
|
||||
pub async fn append_to_layer(
|
||||
&self,
|
||||
layer_name: &str,
|
||||
@@ -840,25 +706,13 @@ impl Workspace {
|
||||
}
|
||||
|
||||
/// Check if a file exists.
|
||||
///
|
||||
/// When multi-scope reads are configured, checks across all read scopes.
|
||||
pub async fn exists(&self, path: &str) -> Result<bool, WorkspaceError> {
|
||||
let path = normalize_path(path);
|
||||
let result = if self.is_multi_scope() && is_identity_path(&path) {
|
||||
// Identity files only checked in primary scope.
|
||||
self.storage
|
||||
.get_document_by_path(&self.user_id, self.agent_id, &path)
|
||||
.await
|
||||
} else if self.is_multi_scope() {
|
||||
self.storage
|
||||
.get_document_by_path_multi(&self.read_user_ids, self.agent_id, &path)
|
||||
.await
|
||||
} else {
|
||||
self.storage
|
||||
.get_document_by_path(&self.user_id, self.agent_id, &path)
|
||||
.await
|
||||
};
|
||||
match result {
|
||||
match self
|
||||
.storage
|
||||
.get_document_by_path(&self.user_id, self.agent_id, &path)
|
||||
.await
|
||||
{
|
||||
Ok(_) => Ok(true),
|
||||
Err(WorkspaceError::DocumentNotFound { .. }) => Ok(false),
|
||||
Err(e) => Err(e),
|
||||
@@ -893,55 +747,16 @@ impl Workspace {
|
||||
/// ```
|
||||
pub async fn list(&self, directory: &str) -> Result<Vec<WorkspaceEntry>, WorkspaceError> {
|
||||
let directory = normalize_directory(directory);
|
||||
if self.is_multi_scope() {
|
||||
// Iterate per-scope rather than using list_directory_multi because
|
||||
// we need to filter identity paths from secondary scopes only — the
|
||||
// merged _multi result loses scope attribution.
|
||||
let primary = self
|
||||
.storage
|
||||
.list_directory(&self.user_id, self.agent_id, &directory)
|
||||
.await?;
|
||||
let mut all_entries = primary;
|
||||
for scope in &self.read_user_ids[1..] {
|
||||
let entries = self
|
||||
.storage
|
||||
.list_directory(scope, self.agent_id, &directory)
|
||||
.await?;
|
||||
all_entries.extend(entries.into_iter().filter(|e| !is_identity_path(&e.path)));
|
||||
}
|
||||
Ok(merge_workspace_entries(all_entries))
|
||||
} else {
|
||||
self.storage
|
||||
.list_directory(&self.user_id, self.agent_id, &directory)
|
||||
.await
|
||||
}
|
||||
self.storage
|
||||
.list_directory(&self.user_id, self.agent_id, &directory)
|
||||
.await
|
||||
}
|
||||
|
||||
/// List all files recursively (flat list of all paths).
|
||||
///
|
||||
/// When multi-scope reads are configured, lists across all read scopes.
|
||||
pub async fn list_all(&self) -> Result<Vec<String>, WorkspaceError> {
|
||||
if self.is_multi_scope() {
|
||||
// Iterate per-scope rather than using list_all_paths_multi because
|
||||
// we need to filter identity paths from secondary scopes only.
|
||||
// Primary scope: all paths. Secondary scopes: filter identity paths.
|
||||
let mut all_paths = self
|
||||
.storage
|
||||
.list_all_paths(&self.user_id, self.agent_id)
|
||||
.await?;
|
||||
for scope in &self.read_user_ids[1..] {
|
||||
let paths = self.storage.list_all_paths(scope, self.agent_id).await?;
|
||||
all_paths.extend(paths.into_iter().filter(|p| !is_identity_path(p)));
|
||||
}
|
||||
// Deduplicate and sort
|
||||
all_paths.sort();
|
||||
all_paths.dedup();
|
||||
Ok(all_paths)
|
||||
} else {
|
||||
self.storage
|
||||
.list_all_paths(&self.user_id, self.agent_id)
|
||||
.await
|
||||
}
|
||||
self.storage
|
||||
.list_all_paths(&self.user_id, self.agent_id)
|
||||
.await
|
||||
}
|
||||
|
||||
// ==================== Convenience Methods ====================
|
||||
@@ -976,7 +791,7 @@ impl Workspace {
|
||||
/// comments, which the heartbeat runner treats as "effectively empty"
|
||||
/// and skips the LLM call.
|
||||
pub async fn heartbeat_checklist(&self) -> Result<Option<String>, WorkspaceError> {
|
||||
match self.read_primary(paths::HEARTBEAT).await {
|
||||
match self.read(paths::HEARTBEAT).await {
|
||||
Ok(doc) => Ok(Some(doc.content)),
|
||||
Err(WorkspaceError::DocumentNotFound { .. }) => Ok(Some(HEARTBEAT_SEED.to_string())),
|
||||
Err(e) => Err(e),
|
||||
@@ -984,29 +799,7 @@ impl Workspace {
|
||||
}
|
||||
|
||||
/// Helper to read or create a file.
|
||||
///
|
||||
/// When multi-scope reads are configured, checks all read scopes before
|
||||
/// creating. If the file exists in any scope, returns it. If not found in
|
||||
/// any scope, creates it in the primary (write) scope.
|
||||
///
|
||||
/// **Important:** In multi-scope mode, the returned document may belong to
|
||||
/// a secondary scope. Callers that intend to **write** to the document
|
||||
/// (via `update_document(doc.id, ...)`) must NOT use this method — use
|
||||
/// `storage.get_or_create_document_by_path(&self.user_id, ...)` instead
|
||||
/// to guarantee writes target the primary scope. See `append_memory` for
|
||||
/// the correct pattern.
|
||||
async fn read_or_create(&self, path: &str) -> Result<MemoryDocument, WorkspaceError> {
|
||||
if self.is_multi_scope() {
|
||||
match self
|
||||
.storage
|
||||
.get_document_by_path_multi(&self.read_user_ids, self.agent_id, path)
|
||||
.await
|
||||
{
|
||||
Ok(doc) => return Ok(doc),
|
||||
Err(WorkspaceError::DocumentNotFound { .. }) => {}
|
||||
Err(e) => return Err(e),
|
||||
}
|
||||
}
|
||||
self.storage
|
||||
.get_or_create_document_by_path(&self.user_id, self.agent_id, path)
|
||||
.await
|
||||
@@ -1018,18 +811,9 @@ impl Workspace {
|
||||
///
|
||||
/// This is for important facts, decisions, and preferences worth
|
||||
/// remembering long-term.
|
||||
///
|
||||
/// Uses `get_or_create_document_by_path` with the primary `user_id`
|
||||
/// instead of `self.memory()` to guarantee writes always target the
|
||||
/// primary (write) scope. `self.memory()` delegates to `read_or_create`,
|
||||
/// which in multi-scope mode may return a document owned by a secondary
|
||||
/// scope; writing to that document by UUID would violate write isolation.
|
||||
pub async fn append_memory(&self, entry: &str) -> Result<(), WorkspaceError> {
|
||||
// Always get/create in the primary scope to preserve write isolation.
|
||||
let doc = self
|
||||
.storage
|
||||
.get_or_create_document_by_path(&self.user_id, self.agent_id, paths::MEMORY)
|
||||
.await?;
|
||||
// Use double newline for memory entries (semantic separation)
|
||||
let doc = self.memory().await?;
|
||||
let new_content = if doc.content.is_empty() {
|
||||
entry.to_string()
|
||||
} else {
|
||||
@@ -1121,16 +905,9 @@ impl Workspace {
|
||||
// Safety net: if `profile_onboarding_completed` was already set (the
|
||||
// LLM completed onboarding but forgot to delete BOOTSTRAP.md), skip
|
||||
// injection to avoid repeating the first-run ritual.
|
||||
//
|
||||
// Identity and config files use read_primary() to prevent cross-scope
|
||||
// bleed in multi-scope workspaces. Without this, a user with read access
|
||||
// to other scopes could silently inherit another user's identity if their
|
||||
// own copy is missing — the agent would present as the wrong person.
|
||||
// Memory files (MEMORY.md, daily logs) intentionally use multi-scope
|
||||
// read() since sharing memory across scopes is a feature.
|
||||
let bootstrap_injected = if self.is_bootstrap_completed() {
|
||||
if self
|
||||
.read_primary(paths::BOOTSTRAP)
|
||||
.read(paths::BOOTSTRAP)
|
||||
.await
|
||||
.is_ok_and(|d| !d.content.is_empty())
|
||||
{
|
||||
@@ -1140,7 +917,7 @@ impl Workspace {
|
||||
);
|
||||
}
|
||||
false
|
||||
} else if let Ok(doc) = self.read_primary(paths::BOOTSTRAP).await
|
||||
} else if let Ok(doc) = self.read(paths::BOOTSTRAP).await
|
||||
&& !doc.content.is_empty()
|
||||
{
|
||||
parts.push(format!("## First-Run Bootstrap\n\n{}", doc.content));
|
||||
@@ -1149,8 +926,7 @@ impl Workspace {
|
||||
false
|
||||
};
|
||||
|
||||
// Load identity files in order of importance.
|
||||
// These MUST use read_primary() — see comment above.
|
||||
// Load identity files in order of importance
|
||||
let identity_files = [
|
||||
(paths::AGENTS, "## Agent Instructions"),
|
||||
(paths::SOUL, "## Core Values"),
|
||||
@@ -1159,7 +935,7 @@ impl Workspace {
|
||||
];
|
||||
|
||||
for (path, header) in identity_files {
|
||||
if let Ok(doc) = self.read_primary(path).await
|
||||
if let Ok(doc) = self.read(path).await
|
||||
&& !doc.content.is_empty()
|
||||
{
|
||||
parts.push(format!("{}\n\n{}", header, doc.content));
|
||||
@@ -1168,8 +944,7 @@ impl Workspace {
|
||||
|
||||
// Tool notes: environment-specific guidance the agent or user has written.
|
||||
// TOOLS.md does not control tool availability; it is guidance only.
|
||||
// Uses read_primary() — tool config is per-user, not inherited.
|
||||
if let Ok(doc) = self.read_primary(paths::TOOLS).await
|
||||
if let Ok(doc) = self.read(paths::TOOLS).await
|
||||
&& !doc.content.is_empty()
|
||||
{
|
||||
parts.push(format!("## Tool Notes\n\n{}", doc.content));
|
||||
@@ -1460,8 +1235,6 @@ impl Workspace {
|
||||
}
|
||||
|
||||
/// Search with custom configuration.
|
||||
///
|
||||
/// When multi-scope reads are configured, searches across all read scopes.
|
||||
pub async fn search_with_config(
|
||||
&self,
|
||||
query: &str,
|
||||
@@ -1481,46 +1254,15 @@ impl Workspace {
|
||||
None
|
||||
};
|
||||
|
||||
if self.is_multi_scope() {
|
||||
let results = self
|
||||
.storage
|
||||
.hybrid_search_multi(
|
||||
&self.read_user_ids,
|
||||
self.agent_id,
|
||||
query,
|
||||
embedding.as_deref(),
|
||||
&config,
|
||||
)
|
||||
.await?;
|
||||
// Post-filter: exclude identity documents from secondary scopes.
|
||||
// Collect document IDs that are identity paths in secondary scopes.
|
||||
let mut excluded_doc_ids = std::collections::HashSet::new();
|
||||
for result in &results {
|
||||
if is_identity_path(&result.document_path) {
|
||||
// Check if this document belongs to a secondary scope
|
||||
match self.storage.get_document_by_id(result.document_id).await {
|
||||
Ok(doc) if doc.user_id != self.user_id => {
|
||||
excluded_doc_ids.insert(result.document_id);
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(results
|
||||
.into_iter()
|
||||
.filter(|r| !excluded_doc_ids.contains(&r.document_id))
|
||||
.collect())
|
||||
} else {
|
||||
self.storage
|
||||
.hybrid_search(
|
||||
&self.user_id,
|
||||
self.agent_id,
|
||||
query,
|
||||
embedding.as_deref(),
|
||||
&config,
|
||||
)
|
||||
.await
|
||||
}
|
||||
self.storage
|
||||
.hybrid_search(
|
||||
&self.user_id,
|
||||
self.agent_id,
|
||||
query,
|
||||
embedding.as_deref(),
|
||||
&config,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
// ==================== Indexing ====================
|
||||
@@ -1581,13 +1323,13 @@ impl Workspace {
|
||||
// Check freshness BEFORE seeding identity files, otherwise the
|
||||
// seeded files make the workspace look non-fresh and BOOTSTRAP.md
|
||||
// never gets created.
|
||||
let is_fresh_workspace = if self.read_primary(paths::BOOTSTRAP).await.is_ok() {
|
||||
let is_fresh_workspace = if self.read(paths::BOOTSTRAP).await.is_ok() {
|
||||
false // BOOTSTRAP already exists
|
||||
} else {
|
||||
let (agents_res, soul_res, user_res) = tokio::join!(
|
||||
self.read_primary(paths::AGENTS),
|
||||
self.read_primary(paths::SOUL),
|
||||
self.read_primary(paths::USER),
|
||||
self.read(paths::AGENTS),
|
||||
self.read(paths::SOUL),
|
||||
self.read(paths::USER),
|
||||
);
|
||||
matches!(agents_res, Err(WorkspaceError::DocumentNotFound { .. }))
|
||||
&& matches!(soul_res, Err(WorkspaceError::DocumentNotFound { .. }))
|
||||
@@ -1596,10 +1338,8 @@ impl Workspace {
|
||||
|
||||
let mut count = 0;
|
||||
for (path, content) in seed_files {
|
||||
// Skip files that already exist in the primary scope (never overwrite user edits).
|
||||
// Uses read_primary to avoid false positives from secondary scopes —
|
||||
// a file in another scope should not suppress seeding in this scope.
|
||||
match self.read_primary(path).await {
|
||||
// Skip files that already exist (never overwrite user edits)
|
||||
match self.read(path).await {
|
||||
Ok(_) => continue,
|
||||
Err(WorkspaceError::DocumentNotFound { .. }) => {}
|
||||
Err(e) => {
|
||||
@@ -1620,8 +1360,7 @@ impl Workspace {
|
||||
// may already have a profile from a previous install and doesn't need
|
||||
// onboarding). This prevents existing users from getting a spurious
|
||||
// first-run ritual after upgrading.
|
||||
// Uses read_primary() to avoid false positives from secondary scopes.
|
||||
let has_profile = self.read_primary(paths::PROFILE).await.is_ok_and(|d| {
|
||||
let has_profile = self.read(paths::PROFILE).await.is_ok_and(|d| {
|
||||
!d.content.trim().is_empty()
|
||||
&& serde_json::from_str::<crate::profile::PsychographicProfile>(&d.content).is_ok()
|
||||
});
|
||||
@@ -2052,67 +1791,4 @@ mod seed_tests {
|
||||
"BOOTSTRAP.md should NOT have been seeded with existing profile"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_default_single_scope() {
|
||||
// Verify backward compatibility: default workspace has single read scope
|
||||
// matching user_id.
|
||||
let user_id = "alice";
|
||||
let read_user_ids = [user_id.to_string()];
|
||||
assert_eq!(read_user_ids.len(), 1);
|
||||
assert_eq!(read_user_ids[0], user_id);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_additional_read_scopes() {
|
||||
// Verify that additional read scopes are added correctly.
|
||||
let user_id = "alice".to_string();
|
||||
let mut read_user_ids = Vec::from([user_id.clone()]);
|
||||
|
||||
// Simulate with_additional_read_scopes logic
|
||||
let scopes = ["shared", "team"];
|
||||
for scope in scopes {
|
||||
let s = scope.to_string();
|
||||
if !read_user_ids.contains(&s) {
|
||||
read_user_ids.push(s);
|
||||
}
|
||||
}
|
||||
|
||||
assert_eq!(read_user_ids.len(), 3);
|
||||
assert_eq!(read_user_ids[0], "alice");
|
||||
assert_eq!(read_user_ids[1], "shared");
|
||||
assert_eq!(read_user_ids[2], "team");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_additional_read_scopes_dedup() {
|
||||
// Verify that duplicate scopes are ignored.
|
||||
let user_id = "alice".to_string();
|
||||
let mut read_user_ids = Vec::from([user_id.clone()]);
|
||||
|
||||
let scopes = ["shared", "alice", "shared"];
|
||||
for scope in scopes {
|
||||
let s = scope.to_string();
|
||||
if !read_user_ids.contains(&s) {
|
||||
read_user_ids.push(s);
|
||||
}
|
||||
}
|
||||
|
||||
assert_eq!(read_user_ids.len(), 2);
|
||||
assert_eq!(read_user_ids[0], "alice");
|
||||
assert_eq!(read_user_ids[1], "shared");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_is_multi_scope_logic() {
|
||||
// Test the multi-scope detection logic: > 1 means multi-scope
|
||||
let single_count = 1_usize;
|
||||
let multi_count = 2_usize;
|
||||
|
||||
// Single scope: not multi
|
||||
assert!(single_count <= 1);
|
||||
|
||||
// Multi scope: is multi
|
||||
assert!(multi_count > 1);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -502,203 +502,4 @@ impl Repository {
|
||||
})
|
||||
.collect())
|
||||
}
|
||||
|
||||
// ==================== Multi-scope search (optimized SQL) ====================
|
||||
|
||||
/// Hybrid search across multiple user scopes with efficient SQL.
|
||||
///
|
||||
/// Uses `user_id = ANY($1::text[])` instead of N separate queries.
|
||||
pub async fn hybrid_search_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
query: &str,
|
||||
embedding: Option<&[f32]>,
|
||||
config: &SearchConfig,
|
||||
) -> Result<Vec<SearchResult>, WorkspaceError> {
|
||||
let fts_results = if config.use_fts {
|
||||
self.fts_search_multi(user_ids, agent_id, query, config.pre_fusion_limit)
|
||||
.await?
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
|
||||
let vector_results = if config.use_vector {
|
||||
if let Some(embedding) = embedding {
|
||||
self.vector_search_multi(user_ids, agent_id, embedding, config.pre_fusion_limit)
|
||||
.await?
|
||||
} else {
|
||||
Vec::new()
|
||||
}
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
|
||||
Ok(fuse_results(fts_results, vector_results, config))
|
||||
}
|
||||
|
||||
/// FTS search across multiple user scopes.
|
||||
async fn fts_search_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
query: &str,
|
||||
limit: usize,
|
||||
) -> Result<Vec<RankedResult>, WorkspaceError> {
|
||||
let conn = self.conn().await?;
|
||||
|
||||
let rows = conn
|
||||
.query(
|
||||
r#"
|
||||
SELECT c.id as chunk_id, c.document_id, d.path as document_path,
|
||||
c.content,
|
||||
ts_rank_cd(c.content_tsv, plainto_tsquery('english', $3)) as rank
|
||||
FROM memory_chunks c
|
||||
JOIN memory_documents d ON d.id = c.document_id
|
||||
WHERE d.user_id = ANY($1::text[]) AND d.agent_id IS NOT DISTINCT FROM $2
|
||||
AND c.content_tsv @@ plainto_tsquery('english', $3)
|
||||
ORDER BY rank DESC
|
||||
LIMIT $4
|
||||
"#,
|
||||
&[&user_ids, &agent_id, &query, &(limit as i64)],
|
||||
)
|
||||
.await
|
||||
.map_err(|e| WorkspaceError::SearchFailed {
|
||||
reason: format!("FTS multi-scope query failed: {}", e),
|
||||
})?;
|
||||
|
||||
Ok(rows
|
||||
.iter()
|
||||
.enumerate()
|
||||
.map(|(i, row)| RankedResult {
|
||||
chunk_id: row.get("chunk_id"),
|
||||
document_id: row.get("document_id"),
|
||||
document_path: row.get("document_path"),
|
||||
content: row.get("content"),
|
||||
rank: (i + 1) as u32,
|
||||
})
|
||||
.collect())
|
||||
}
|
||||
|
||||
/// Vector search across multiple user scopes.
|
||||
async fn vector_search_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
embedding: &[f32],
|
||||
limit: usize,
|
||||
) -> Result<Vec<RankedResult>, WorkspaceError> {
|
||||
let conn = self.conn().await?;
|
||||
let embedding_vec = Vector::from(embedding.to_vec());
|
||||
|
||||
let rows = conn
|
||||
.query(
|
||||
r#"
|
||||
SELECT c.id as chunk_id, c.document_id, d.path as document_path,
|
||||
c.content, 1 - (c.embedding <=> $3) as similarity
|
||||
FROM memory_chunks c
|
||||
JOIN memory_documents d ON d.id = c.document_id
|
||||
WHERE d.user_id = ANY($1::text[]) AND d.agent_id IS NOT DISTINCT FROM $2
|
||||
AND c.embedding IS NOT NULL
|
||||
ORDER BY c.embedding <=> $3
|
||||
LIMIT $4
|
||||
"#,
|
||||
&[&user_ids, &agent_id, &embedding_vec, &(limit as i64)],
|
||||
)
|
||||
.await
|
||||
.map_err(|e| WorkspaceError::SearchFailed {
|
||||
reason: format!("Vector multi-scope query failed: {}", e),
|
||||
})?;
|
||||
|
||||
Ok(rows
|
||||
.iter()
|
||||
.enumerate()
|
||||
.map(|(i, row)| RankedResult {
|
||||
chunk_id: row.get("chunk_id"),
|
||||
document_id: row.get("document_id"),
|
||||
document_path: row.get("document_path"),
|
||||
content: row.get("content"),
|
||||
rank: (i + 1) as u32,
|
||||
})
|
||||
.collect())
|
||||
}
|
||||
|
||||
/// List all file paths across multiple user scopes with a single query.
|
||||
pub async fn list_all_paths_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
) -> Result<Vec<String>, WorkspaceError> {
|
||||
let conn = self.conn().await?;
|
||||
|
||||
let rows = conn
|
||||
.query(
|
||||
r#"
|
||||
SELECT DISTINCT path FROM memory_documents
|
||||
WHERE user_id = ANY($1::text[]) AND agent_id IS NOT DISTINCT FROM $2
|
||||
ORDER BY path
|
||||
"#,
|
||||
&[&user_ids, &agent_id],
|
||||
)
|
||||
.await
|
||||
.map_err(|e| WorkspaceError::SearchFailed {
|
||||
reason: format!("List paths multi-scope failed: {}", e),
|
||||
})?;
|
||||
|
||||
Ok(rows.iter().map(|row| row.get("path")).collect())
|
||||
}
|
||||
|
||||
/// Get a document by path across multiple user scopes.
|
||||
///
|
||||
/// Returns the first match (ordered by the input user_ids priority).
|
||||
pub async fn get_document_by_path_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
path: &str,
|
||||
) -> Result<MemoryDocument, WorkspaceError> {
|
||||
let conn = self.conn().await?;
|
||||
|
||||
let row = conn
|
||||
.query_opt(
|
||||
r#"
|
||||
SELECT id, user_id, agent_id, path, content,
|
||||
created_at, updated_at, metadata
|
||||
FROM memory_documents
|
||||
WHERE user_id = ANY($1::text[]) AND agent_id IS NOT DISTINCT FROM $2 AND path = $3
|
||||
ORDER BY array_position($1::text[], user_id)
|
||||
LIMIT 1
|
||||
"#,
|
||||
&[&user_ids, &agent_id, &path],
|
||||
)
|
||||
.await
|
||||
.map_err(|e| WorkspaceError::SearchFailed {
|
||||
reason: format!("get_document_by_path_multi failed: {}", e),
|
||||
})?;
|
||||
|
||||
match row {
|
||||
Some(row) => Ok(self.row_to_document(&row)),
|
||||
None => Err(WorkspaceError::DocumentNotFound {
|
||||
doc_type: path.to_string(),
|
||||
user_id: format!("[{}]", user_ids.join(", ")),
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
/// List directory contents across multiple user scopes.
|
||||
///
|
||||
/// Iterates per scope and merges results. A future migration could add an
|
||||
/// optimised SQL function, at which point this method can call it directly.
|
||||
pub async fn list_directory_multi(
|
||||
&self,
|
||||
user_ids: &[String],
|
||||
agent_id: Option<Uuid>,
|
||||
directory: &str,
|
||||
) -> Result<Vec<WorkspaceEntry>, WorkspaceError> {
|
||||
let mut all_entries = Vec::new();
|
||||
for uid in user_ids {
|
||||
all_entries.extend(self.list_directory(uid, agent_id, directory).await?);
|
||||
}
|
||||
Ok(crate::workspace::merge_workspace_entries(all_entries))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,195 +0,0 @@
|
||||
//! Tests for identity file scope isolation in multi-scope workspaces.
|
||||
//!
|
||||
//! When a workspace has multiple read scopes (e.g., Andrew can read from
|
||||
//! "andrew", "grace", "household"), identity files (SOUL.md, USER.md,
|
||||
//! IDENTITY.md, AGENTS.md) must ONLY come from the primary scope.
|
||||
//!
|
||||
//! Multi-scope reads are designed for memory sharing (MEMORY.md, daily logs),
|
||||
//! not identity inheritance. Silently inheriting identity from another scope
|
||||
//! is a correctness and security issue — the agent would present itself as
|
||||
//! the wrong user.
|
||||
//!
|
||||
//! These tests verify that:
|
||||
//! 1. Identity files are read from primary scope only
|
||||
//! 2. If the primary scope's identity file is missing, it's absent from the
|
||||
//! system prompt — never falls back to another scope
|
||||
//! 3. Memory files (MEMORY.md) still benefit from multi-scope reads
|
||||
#![cfg(feature = "libsql")]
|
||||
|
||||
use std::sync::Arc;
|
||||
|
||||
use ironclaw::db::Database;
|
||||
use ironclaw::db::libsql::LibSqlBackend;
|
||||
use ironclaw::workspace::{Workspace, paths};
|
||||
|
||||
async fn setup() -> (Arc<dyn Database>, tempfile::TempDir) {
|
||||
let dir = tempfile::tempdir().expect("create temp dir");
|
||||
let db_path = dir.path().join("test.db");
|
||||
let backend = LibSqlBackend::new_local(&db_path).await.expect("create db");
|
||||
backend.run_migrations().await.expect("run migrations");
|
||||
let db: Arc<dyn Database> = Arc::new(backend);
|
||||
(db, dir)
|
||||
}
|
||||
|
||||
/// Seed a document into a specific user's workspace scope.
|
||||
async fn seed(db: &Arc<dyn Database>, user_id: &str, path: &str, content: &str) {
|
||||
let ws = Workspace::new_with_db(user_id, db.clone());
|
||||
ws.write(path, content)
|
||||
.await
|
||||
.unwrap_or_else(|e| panic!("Failed to seed {path} for {user_id}: {e}"));
|
||||
}
|
||||
|
||||
// ─── Test 1: Primary scope identity appears in system prompt ───────────
|
||||
|
||||
#[tokio::test]
|
||||
async fn system_prompt_uses_primary_scope_identity() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Seed Alice's identity files in her own scope
|
||||
seed(&db, "alice", paths::SOUL, "Alice is kind and curious.").await;
|
||||
seed(
|
||||
&db,
|
||||
"alice",
|
||||
paths::USER,
|
||||
"You are talking to Alice, a software engineer.",
|
||||
)
|
||||
.await;
|
||||
|
||||
// Seed Bob's identity files in his scope
|
||||
seed(&db, "bob", paths::SOUL, "Bob is analytical and precise.").await;
|
||||
seed(
|
||||
&db,
|
||||
"bob",
|
||||
paths::USER,
|
||||
"You are talking to Bob, a marine biologist.",
|
||||
)
|
||||
.await;
|
||||
|
||||
// Create Alice's workspace WITH multi-scope reads including Bob
|
||||
let ws = Workspace::new_with_db("alice", db.clone())
|
||||
.with_additional_read_scopes(vec!["bob".to_string()]);
|
||||
|
||||
let prompt = ws
|
||||
.system_prompt_for_context(false)
|
||||
.await
|
||||
.expect("system_prompt_for_context failed");
|
||||
|
||||
// Alice's identity must appear
|
||||
assert!(
|
||||
prompt.contains("Alice is kind and curious"),
|
||||
"Primary scope SOUL.md should appear in system prompt.\nPrompt:\n{prompt}"
|
||||
);
|
||||
assert!(
|
||||
prompt.contains("Alice, a software engineer"),
|
||||
"Primary scope USER.md should appear in system prompt.\nPrompt:\n{prompt}"
|
||||
);
|
||||
|
||||
// Bob's identity must NOT appear
|
||||
assert!(
|
||||
!prompt.contains("Bob is analytical"),
|
||||
"Secondary scope SOUL.md must NOT appear in system prompt.\nPrompt:\n{prompt}"
|
||||
);
|
||||
assert!(
|
||||
!prompt.contains("Bob, a marine biologist"),
|
||||
"Secondary scope USER.md must NOT appear in system prompt.\nPrompt:\n{prompt}"
|
||||
);
|
||||
}
|
||||
|
||||
// ─── Test 2: Missing primary identity does NOT fall back to other scope ─
|
||||
|
||||
#[tokio::test]
|
||||
async fn missing_primary_identity_does_not_fallback_to_other_scope() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Only seed Bob's identity — Alice has no identity files
|
||||
seed(&db, "bob", paths::SOUL, "Bob is analytical and precise.").await;
|
||||
seed(
|
||||
&db,
|
||||
"bob",
|
||||
paths::USER,
|
||||
"You are talking to Bob, a marine biologist.",
|
||||
)
|
||||
.await;
|
||||
|
||||
// Create Alice's workspace with multi-scope reads including Bob
|
||||
let ws = Workspace::new_with_db("alice", db.clone())
|
||||
.with_additional_read_scopes(vec!["bob".to_string()]);
|
||||
|
||||
let prompt = ws
|
||||
.system_prompt_for_context(false)
|
||||
.await
|
||||
.expect("system_prompt_for_context failed");
|
||||
|
||||
// Bob's identity must NOT appear — Alice's missing identity should stay missing,
|
||||
// not silently inherit from Bob's scope
|
||||
assert!(
|
||||
!prompt.contains("Bob"),
|
||||
"When primary scope identity is missing, must NOT fall back to secondary scope.\n\
|
||||
This would cause the agent to present itself as the wrong user.\nPrompt:\n{prompt}"
|
||||
);
|
||||
}
|
||||
|
||||
// ─── Test 3: MEMORY.md still benefits from multi-scope reads ────────────
|
||||
|
||||
#[tokio::test]
|
||||
async fn memory_files_still_use_multi_scope_reads() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Seed shared memory in the "shared" scope (not Alice's primary)
|
||||
seed(
|
||||
&db,
|
||||
"shared",
|
||||
paths::MEMORY,
|
||||
"Shared grocery list: milk, eggs, bread.",
|
||||
)
|
||||
.await;
|
||||
|
||||
// Create Alice's workspace with read access to shared scope
|
||||
let ws = Workspace::new_with_db("alice", db.clone())
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
|
||||
let prompt = ws
|
||||
.system_prompt_for_context(false)
|
||||
.await
|
||||
.expect("system_prompt_for_context failed");
|
||||
|
||||
// Shared memory SHOULD appear — multi-scope reads are correct for memory
|
||||
assert!(
|
||||
prompt.contains("grocery list"),
|
||||
"MEMORY.md should still use multi-scope reads.\nPrompt:\n{prompt}"
|
||||
);
|
||||
}
|
||||
|
||||
// ─── Test 4: All identity files are scope-isolated ──────────────────────
|
||||
|
||||
#[tokio::test]
|
||||
async fn all_identity_files_are_scope_isolated() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Seed identity files ONLY in the "other" scope, not in Alice's
|
||||
seed(&db, "other", paths::AGENTS, "You are Other's agent.").await;
|
||||
seed(&db, "other", paths::SOUL, "Other's soul values.").await;
|
||||
seed(&db, "other", paths::USER, "You are talking to Other.").await;
|
||||
seed(&db, "other", paths::IDENTITY, "Other's identity.").await;
|
||||
|
||||
// Also seed BOOTSTRAP.md and TOOLS.md in other scope
|
||||
seed(&db, "other", "BOOTSTRAP.md", "Other's bootstrap.").await;
|
||||
seed(&db, "other", "TOOLS.md", "Other's tool notes.").await;
|
||||
|
||||
// Create Alice's workspace with read access to "other"
|
||||
let ws = Workspace::new_with_db("alice", db.clone())
|
||||
.with_additional_read_scopes(vec!["other".to_string()]);
|
||||
|
||||
let prompt = ws
|
||||
.system_prompt_for_context(false)
|
||||
.await
|
||||
.expect("system_prompt_for_context failed");
|
||||
|
||||
// None of Other's identity/config files should appear
|
||||
assert!(
|
||||
!prompt.contains("Other"),
|
||||
"No identity or config files from secondary scope should appear.\n\
|
||||
Every identity file (AGENTS.md, SOUL.md, USER.md, IDENTITY.md, \
|
||||
BOOTSTRAP.md, TOOLS.md) must read from primary scope only.\nPrompt:\n{prompt}"
|
||||
);
|
||||
}
|
||||
@@ -1,451 +0,0 @@
|
||||
#![cfg(feature = "libsql")]
|
||||
//! Integration tests for multi-scope workspace reads using file-backed libSQL.
|
||||
//!
|
||||
//! Guards the PR2 contract: workspaces can read from multiple user scopes
|
||||
//! while writes remain isolated to the primary scope.
|
||||
|
||||
use std::sync::Arc;
|
||||
|
||||
use ironclaw::db::Database;
|
||||
use ironclaw::db::libsql::LibSqlBackend;
|
||||
use ironclaw::workspace::Workspace;
|
||||
|
||||
async fn setup() -> (Arc<dyn Database>, tempfile::TempDir) {
|
||||
let dir = tempfile::tempdir().expect("create temp dir");
|
||||
let db_path = dir.path().join("test.db");
|
||||
let backend = LibSqlBackend::new_local(&db_path).await.expect("create db");
|
||||
backend.run_migrations().await.expect("run migrations");
|
||||
let db: Arc<dyn Database> = Arc::new(backend);
|
||||
(db, dir)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_across_scopes() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Write docs as the "shared" user
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write("docs/team-standup.md", "Team standup notes from Monday")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice's workspace with "shared" as an additional read scope
|
||||
let ws_alice = Workspace::new_with_db("alice", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
|
||||
// Alice can read shared docs
|
||||
let doc = ws_alice
|
||||
.read("docs/team-standup.md")
|
||||
.await
|
||||
.expect("cross-scope read failed");
|
||||
assert_eq!(doc.content, "Team standup notes from Monday");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_stays_in_primary_scope() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Alice has "shared" as a read scope
|
||||
let ws_alice = Workspace::new_with_db("alice", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
|
||||
// Alice writes a personal note
|
||||
ws_alice
|
||||
.write("notes/personal.md", "Alice's private note")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
// The "shared" workspace should NOT see Alice's note
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
let result = ws_shared.read("notes/personal.md").await;
|
||||
assert!(result.is_err(), "Shared scope should not see Alice's note");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn list_paths_merges_across_scopes() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Write as alice
|
||||
let ws_alice_plain = Workspace::new_with_db("alice", Arc::clone(&db));
|
||||
ws_alice_plain
|
||||
.write("notes/personal.md", "My notes")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
// Write as shared
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write("docs/shared-doc.md", "Shared document")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice with multi-scope should see both
|
||||
let ws_alice = Workspace::new_with_db("alice", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
|
||||
let all_paths = ws_alice.list_all().await.expect("list_all failed");
|
||||
assert!(
|
||||
all_paths.contains(&"notes/personal.md".to_string()),
|
||||
"Should contain alice's note: {:?}",
|
||||
all_paths
|
||||
);
|
||||
assert!(
|
||||
all_paths.contains(&"docs/shared-doc.md".to_string()),
|
||||
"Should contain shared doc: {:?}",
|
||||
all_paths
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn list_directory_merges_across_scopes() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Alice writes to docs/
|
||||
let ws_alice_plain = Workspace::new_with_db("alice", Arc::clone(&db));
|
||||
ws_alice_plain
|
||||
.write("docs/alice-doc.md", "Alice's doc")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
// Shared writes to docs/
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write("docs/shared-doc.md", "Shared doc")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice with multi-scope lists docs/
|
||||
let ws_alice = Workspace::new_with_db("alice", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
|
||||
let entries = ws_alice.list("docs").await.expect("list failed");
|
||||
let paths: Vec<&str> = entries.iter().map(|e| e.path.as_str()).collect();
|
||||
assert!(
|
||||
paths.contains(&"docs/alice-doc.md"),
|
||||
"Should contain alice's doc: {:?}",
|
||||
paths
|
||||
);
|
||||
assert!(
|
||||
paths.contains(&"docs/shared-doc.md"),
|
||||
"Should contain shared doc: {:?}",
|
||||
paths
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn search_spans_scopes() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Write searchable content in shared scope
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write(
|
||||
"docs/architecture.md",
|
||||
"The microservice architecture uses gRPC for inter-service communication",
|
||||
)
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Write searchable content in alice scope
|
||||
let ws_alice_plain = Workspace::new_with_db("alice", Arc::clone(&db));
|
||||
ws_alice_plain
|
||||
.write("notes/ideas.md", "Consider switching to GraphQL federation")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
// Alice with multi-scope searches
|
||||
let ws_alice = Workspace::new_with_db("alice", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
|
||||
// Search for content in the shared scope
|
||||
let results = ws_alice
|
||||
.search("microservice architecture gRPC", 10)
|
||||
.await
|
||||
.expect("search failed");
|
||||
assert!(!results.is_empty(), "Should find results from shared scope");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_priority_primary_first() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Write same path in both scopes
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write("config/settings.md", "Shared settings v1")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
let ws_alice_plain = Workspace::new_with_db("alice", Arc::clone(&db));
|
||||
ws_alice_plain
|
||||
.write("config/settings.md", "Alice's settings override")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
// Alice with multi-scope should get her own version (primary scope wins)
|
||||
let ws_alice = Workspace::new_with_db("alice", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
|
||||
let doc = ws_alice
|
||||
.read("config/settings.md")
|
||||
.await
|
||||
.expect("read failed");
|
||||
assert_eq!(
|
||||
doc.content, "Alice's settings override",
|
||||
"Primary scope should take priority"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn exists_spans_scopes() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Write a doc as "shared"
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write("docs/shared-only.md", "Shared content")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice without multi-scope should NOT see it
|
||||
let ws_alice_plain = Workspace::new_with_db("alice", Arc::clone(&db));
|
||||
assert!(
|
||||
!ws_alice_plain
|
||||
.exists("docs/shared-only.md")
|
||||
.await
|
||||
.expect("exists failed"),
|
||||
"Alice without multi-scope should not see shared doc"
|
||||
);
|
||||
|
||||
// Alice with multi-scope should see it
|
||||
let ws_alice = Workspace::new_with_db("alice", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
assert!(
|
||||
ws_alice
|
||||
.exists("docs/shared-only.md")
|
||||
.await
|
||||
.expect("exists failed"),
|
||||
"Alice with multi-scope should see shared doc"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn append_stays_in_primary_scope() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Write a document as "shared"
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write("notes/log.md", "shared original content")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice has "shared" as a read scope and appends to the same path
|
||||
let ws_alice = Workspace::new_with_db("alice", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
ws_alice
|
||||
.append("notes/log.md", "alice appended line")
|
||||
.await
|
||||
.expect("alice append failed");
|
||||
|
||||
// Shared document must be unchanged (write isolation)
|
||||
let shared_doc = ws_shared
|
||||
.read("notes/log.md")
|
||||
.await
|
||||
.expect("shared read failed");
|
||||
assert_eq!(
|
||||
shared_doc.content, "shared original content",
|
||||
"Append must not modify the secondary scope's document"
|
||||
);
|
||||
|
||||
// Alice should have her own copy with the appended content
|
||||
let ws_alice_plain = Workspace::new_with_db("alice", Arc::clone(&db));
|
||||
let alice_doc = ws_alice_plain
|
||||
.read("notes/log.md")
|
||||
.await
|
||||
.expect("alice read failed");
|
||||
assert_eq!(
|
||||
alice_doc.content, "alice appended line",
|
||||
"Append should create a new document in alice's scope"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn append_memory_stays_in_primary_scope() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
// Write MEMORY.md as "shared"
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write("MEMORY.md", "shared memory baseline")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice has "shared" as a read scope and appends a memory entry
|
||||
let ws_alice = Workspace::new_with_db("alice", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string()]);
|
||||
ws_alice
|
||||
.append_memory("alice remembers this")
|
||||
.await
|
||||
.expect("alice append_memory failed");
|
||||
|
||||
// Shared MEMORY.md must be unchanged
|
||||
let shared_doc = ws_shared
|
||||
.read("MEMORY.md")
|
||||
.await
|
||||
.expect("shared read failed");
|
||||
assert_eq!(
|
||||
shared_doc.content, "shared memory baseline",
|
||||
"append_memory must not modify the secondary scope's document"
|
||||
);
|
||||
|
||||
// Alice should have her own MEMORY.md
|
||||
let ws_alice_plain = Workspace::new_with_db("alice", Arc::clone(&db));
|
||||
let alice_doc = ws_alice_plain
|
||||
.read("MEMORY.md")
|
||||
.await
|
||||
.expect("alice read failed");
|
||||
assert_eq!(
|
||||
alice_doc.content, "alice remembers this",
|
||||
"append_memory should create in alice's scope"
|
||||
);
|
||||
}
|
||||
|
||||
// ==================== Identity isolation tests ====================
|
||||
|
||||
#[tokio::test]
|
||||
async fn identity_files_not_readable_from_secondary_scope() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
let ws_other = Workspace::new_with_db("other-user", Arc::clone(&db));
|
||||
ws_other
|
||||
.write("IDENTITY.md", "I am the other user")
|
||||
.await
|
||||
.expect("write failed");
|
||||
ws_other
|
||||
.write("SOUL.md", "Other user soul overlay")
|
||||
.await
|
||||
.expect("write failed");
|
||||
ws_other
|
||||
.write("USER.md", "Other user profile")
|
||||
.await
|
||||
.expect("write failed");
|
||||
ws_other
|
||||
.write("AGENTS.md", "Other user agent config")
|
||||
.await
|
||||
.expect("write failed");
|
||||
|
||||
let ws_primary = Workspace::new_with_db("primary", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["other-user".to_string()]);
|
||||
|
||||
for path in &["IDENTITY.md", "SOUL.md", "USER.md", "AGENTS.md"] {
|
||||
let result = ws_primary.read(path).await;
|
||||
assert!(
|
||||
result.is_err(),
|
||||
"Primary should NOT read other user's {} via secondary scope",
|
||||
path
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn identity_files_not_in_search_from_secondary_scope() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
let ws_other = Workspace::new_with_db("other-user", Arc::clone(&db));
|
||||
ws_other
|
||||
.write("SOUL.md", "Other user loves xylophone music passionately")
|
||||
.await
|
||||
.expect("write failed");
|
||||
ws_other
|
||||
.write(
|
||||
"notes/music.md",
|
||||
"Other user played xylophone at the concert",
|
||||
)
|
||||
.await
|
||||
.expect("write failed");
|
||||
|
||||
let ws_primary = Workspace::new_with_db("primary", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["other-user".to_string()]);
|
||||
|
||||
let results = ws_primary
|
||||
.search("xylophone", 10)
|
||||
.await
|
||||
.expect("search failed");
|
||||
let has_concert = results.iter().any(|r| r.content.contains("concert"));
|
||||
assert!(
|
||||
has_concert,
|
||||
"Should find non-identity content from secondary scope"
|
||||
);
|
||||
let has_soul = results.iter().any(|r| r.content.contains("passionately"));
|
||||
assert!(
|
||||
!has_soul,
|
||||
"SOUL.md content from secondary scope should not appear in search results"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn identity_files_not_in_list_from_secondary_scope() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
let ws_other = Workspace::new_with_db("other-user", Arc::clone(&db));
|
||||
ws_other
|
||||
.write("IDENTITY.md", "I am the other user")
|
||||
.await
|
||||
.expect("write failed");
|
||||
ws_other
|
||||
.write("notes/shared-note.md", "A shared note")
|
||||
.await
|
||||
.expect("write failed");
|
||||
|
||||
let ws_primary = Workspace::new_with_db("primary", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["other-user".to_string()]);
|
||||
|
||||
let paths = ws_primary.list_all().await.expect("list failed");
|
||||
assert!(
|
||||
!paths.contains(&"IDENTITY.md".to_string()),
|
||||
"IDENTITY.md from secondary scope should not appear"
|
||||
);
|
||||
assert!(
|
||||
paths.contains(&"notes/shared-note.md".to_string()),
|
||||
"Non-identity files should be listed"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn empty_read_scopes_reads_primary_only() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write("docs/note.md", "Shared note")
|
||||
.await
|
||||
.expect("write failed");
|
||||
|
||||
let ws_primary =
|
||||
Workspace::new_with_db("primary", Arc::clone(&db)).with_additional_read_scopes(vec![]);
|
||||
|
||||
let result = ws_primary.read("docs/note.md").await;
|
||||
assert!(
|
||||
result.is_err(),
|
||||
"Empty read scopes should not grant cross-scope access"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn duplicate_read_scopes_handled() {
|
||||
let (db, _dir) = setup().await;
|
||||
|
||||
let ws_shared = Workspace::new_with_db("shared", Arc::clone(&db));
|
||||
ws_shared
|
||||
.write("docs/note.md", "One note")
|
||||
.await
|
||||
.expect("write failed");
|
||||
|
||||
let ws_primary = Workspace::new_with_db("primary", Arc::clone(&db))
|
||||
.with_additional_read_scopes(vec!["shared".to_string(), "shared".to_string()]);
|
||||
|
||||
let doc = ws_primary.read("docs/note.md").await.expect("read failed");
|
||||
assert_eq!(doc.content, "One note");
|
||||
}
|
||||
@@ -407,333 +407,3 @@ async fn test_workspace_system_prompt() {
|
||||
|
||||
cleanup_user(&pool, user_id).await;
|
||||
}
|
||||
|
||||
// ── Multi-scope workspace read tests ──────────────────────────────────
|
||||
//
|
||||
// These exercise the PostgreSQL-optimized `_multi` query paths
|
||||
// (repository.rs) that the libSQL backend covers via default trait impls.
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_multi_scope_read_across_scopes() {
|
||||
let pool = get_pool();
|
||||
if try_connect(&pool).await.is_none() {
|
||||
return;
|
||||
}
|
||||
let shared_id = "ms_shared_read";
|
||||
let alice_id = "ms_alice_read";
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
|
||||
// Write a doc as "shared"
|
||||
let ws_shared = Workspace::new(shared_id, pool.clone());
|
||||
ws_shared
|
||||
.write("docs/team-standup.md", "Team standup notes from Monday")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice with "shared" as an additional read scope
|
||||
let ws_alice = Workspace::new(alice_id, pool.clone())
|
||||
.with_additional_read_scopes(vec![shared_id.to_string()]);
|
||||
|
||||
let doc = ws_alice
|
||||
.read("docs/team-standup.md")
|
||||
.await
|
||||
.expect("cross-scope read failed");
|
||||
assert_eq!(doc.content, "Team standup notes from Monday");
|
||||
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_multi_scope_write_stays_in_primary() {
|
||||
let pool = get_pool();
|
||||
if try_connect(&pool).await.is_none() {
|
||||
return;
|
||||
}
|
||||
let shared_id = "ms_shared_write";
|
||||
let alice_id = "ms_alice_write";
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
|
||||
let ws_alice = Workspace::new(alice_id, pool.clone())
|
||||
.with_additional_read_scopes(vec![shared_id.to_string()]);
|
||||
|
||||
ws_alice
|
||||
.write("notes/personal.md", "Alice's private note")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
// Shared workspace should NOT see Alice's note
|
||||
let ws_shared = Workspace::new(shared_id, pool.clone());
|
||||
let result = ws_shared.read("notes/personal.md").await;
|
||||
assert!(result.is_err(), "Shared scope should not see Alice's note");
|
||||
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_multi_scope_list_all_merges() {
|
||||
let pool = get_pool();
|
||||
if try_connect(&pool).await.is_none() {
|
||||
return;
|
||||
}
|
||||
let shared_id = "ms_shared_list";
|
||||
let alice_id = "ms_alice_list";
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
|
||||
// Write as alice (plain, no multi-scope)
|
||||
let ws_alice_plain = Workspace::new(alice_id, pool.clone());
|
||||
ws_alice_plain
|
||||
.write("notes/personal.md", "My notes")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
// Write as shared
|
||||
let ws_shared = Workspace::new(shared_id, pool.clone());
|
||||
ws_shared
|
||||
.write("docs/shared-doc.md", "Shared document")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice with multi-scope should see both
|
||||
let ws_alice = Workspace::new(alice_id, pool.clone())
|
||||
.with_additional_read_scopes(vec![shared_id.to_string()]);
|
||||
|
||||
let all_paths = ws_alice.list_all().await.expect("list_all failed");
|
||||
assert!(
|
||||
all_paths.contains(&"notes/personal.md".to_string()),
|
||||
"Should contain alice's note: {:?}",
|
||||
all_paths
|
||||
);
|
||||
assert!(
|
||||
all_paths.contains(&"docs/shared-doc.md".to_string()),
|
||||
"Should contain shared doc: {:?}",
|
||||
all_paths
|
||||
);
|
||||
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_multi_scope_list_directory_merges() {
|
||||
let pool = get_pool();
|
||||
if try_connect(&pool).await.is_none() {
|
||||
return;
|
||||
}
|
||||
let shared_id = "ms_shared_dir";
|
||||
let alice_id = "ms_alice_dir";
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
|
||||
let ws_alice_plain = Workspace::new(alice_id, pool.clone());
|
||||
ws_alice_plain
|
||||
.write("docs/alice-doc.md", "Alice's doc")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
let ws_shared = Workspace::new(shared_id, pool.clone());
|
||||
ws_shared
|
||||
.write("docs/shared-doc.md", "Shared doc")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
let ws_alice = Workspace::new(alice_id, pool.clone())
|
||||
.with_additional_read_scopes(vec![shared_id.to_string()]);
|
||||
|
||||
let entries = ws_alice.list("docs").await.expect("list failed");
|
||||
let paths: Vec<&str> = entries.iter().map(|e| e.path.as_str()).collect();
|
||||
assert!(
|
||||
paths.contains(&"docs/alice-doc.md"),
|
||||
"Should contain alice's doc: {:?}",
|
||||
paths
|
||||
);
|
||||
assert!(
|
||||
paths.contains(&"docs/shared-doc.md"),
|
||||
"Should contain shared doc: {:?}",
|
||||
paths
|
||||
);
|
||||
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_multi_scope_read_priority_primary_first() {
|
||||
let pool = get_pool();
|
||||
if try_connect(&pool).await.is_none() {
|
||||
return;
|
||||
}
|
||||
let shared_id = "ms_shared_prio";
|
||||
let alice_id = "ms_alice_prio";
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
|
||||
// Write same path in both scopes
|
||||
let ws_shared = Workspace::new(shared_id, pool.clone());
|
||||
ws_shared
|
||||
.write("config/settings.md", "Shared settings v1")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
let ws_alice_plain = Workspace::new(alice_id, pool.clone());
|
||||
ws_alice_plain
|
||||
.write("config/settings.md", "Alice's settings override")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
// Alice with multi-scope should get her own version (primary scope wins)
|
||||
let ws_alice = Workspace::new(alice_id, pool.clone())
|
||||
.with_additional_read_scopes(vec![shared_id.to_string()]);
|
||||
|
||||
let doc = ws_alice
|
||||
.read("config/settings.md")
|
||||
.await
|
||||
.expect("read failed");
|
||||
assert_eq!(
|
||||
doc.content, "Alice's settings override",
|
||||
"Primary scope should take priority"
|
||||
);
|
||||
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_multi_scope_exists_spans_scopes() {
|
||||
let pool = get_pool();
|
||||
if try_connect(&pool).await.is_none() {
|
||||
return;
|
||||
}
|
||||
let shared_id = "ms_shared_exists";
|
||||
let alice_id = "ms_alice_exists";
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
|
||||
let ws_shared = Workspace::new(shared_id, pool.clone());
|
||||
ws_shared
|
||||
.write("docs/shared-only.md", "Shared content")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice without multi-scope should NOT see it
|
||||
let ws_alice_plain = Workspace::new(alice_id, pool.clone());
|
||||
assert!(
|
||||
!ws_alice_plain
|
||||
.exists("docs/shared-only.md")
|
||||
.await
|
||||
.expect("exists failed"),
|
||||
"Alice without multi-scope should not see shared doc"
|
||||
);
|
||||
|
||||
// Alice with multi-scope should see it
|
||||
let ws_alice = Workspace::new(alice_id, pool.clone())
|
||||
.with_additional_read_scopes(vec![shared_id.to_string()]);
|
||||
assert!(
|
||||
ws_alice
|
||||
.exists("docs/shared-only.md")
|
||||
.await
|
||||
.expect("exists failed"),
|
||||
"Alice with multi-scope should see shared doc"
|
||||
);
|
||||
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_multi_scope_search_spans_scopes() {
|
||||
let pool = get_pool();
|
||||
if try_connect(&pool).await.is_none() {
|
||||
return;
|
||||
}
|
||||
let shared_id = "ms_shared_search";
|
||||
let alice_id = "ms_alice_search";
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
|
||||
let ws_shared = Workspace::new(shared_id, pool.clone());
|
||||
ws_shared
|
||||
.write(
|
||||
"docs/architecture.md",
|
||||
"The microservice architecture uses gRPC for inter-service communication",
|
||||
)
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
let ws_alice_plain = Workspace::new(alice_id, pool.clone());
|
||||
ws_alice_plain
|
||||
.write("notes/ideas.md", "Consider switching to GraphQL federation")
|
||||
.await
|
||||
.expect("alice write failed");
|
||||
|
||||
let ws_alice = Workspace::new(alice_id, pool.clone())
|
||||
.with_additional_read_scopes(vec![shared_id.to_string()]);
|
||||
|
||||
// Search for content in the shared scope
|
||||
let results = ws_alice
|
||||
.search_with_config(
|
||||
"microservice gRPC architecture",
|
||||
SearchConfig::default().fts_only(),
|
||||
)
|
||||
.await
|
||||
.expect("search failed");
|
||||
assert!(!results.is_empty(), "Should find results from shared scope");
|
||||
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_multi_scope_append_stays_in_primary() {
|
||||
let pool = get_pool();
|
||||
if try_connect(&pool).await.is_none() {
|
||||
return;
|
||||
}
|
||||
let shared_id = "ms_shared_append";
|
||||
let alice_id = "ms_alice_append";
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
|
||||
// Write a document as "shared"
|
||||
let ws_shared = Workspace::new(shared_id, pool.clone());
|
||||
ws_shared
|
||||
.write("notes/log.md", "shared original content")
|
||||
.await
|
||||
.expect("shared write failed");
|
||||
|
||||
// Alice has "shared" as a read scope and appends to the same path
|
||||
let ws_alice = Workspace::new(alice_id, pool.clone())
|
||||
.with_additional_read_scopes(vec![shared_id.to_string()]);
|
||||
ws_alice
|
||||
.append("notes/log.md", "alice appended line")
|
||||
.await
|
||||
.expect("alice append failed");
|
||||
|
||||
// Shared document must be unchanged (write isolation)
|
||||
let shared_doc = ws_shared
|
||||
.read("notes/log.md")
|
||||
.await
|
||||
.expect("shared read failed");
|
||||
assert_eq!(
|
||||
shared_doc.content, "shared original content",
|
||||
"Append must not modify the secondary scope's document"
|
||||
);
|
||||
|
||||
// Alice should have her own copy with the appended content
|
||||
let ws_alice_plain = Workspace::new(alice_id, pool.clone());
|
||||
let alice_doc = ws_alice_plain
|
||||
.read("notes/log.md")
|
||||
.await
|
||||
.expect("alice read failed");
|
||||
assert_eq!(
|
||||
alice_doc.content, "alice appended line",
|
||||
"Append should create a new document in alice's scope"
|
||||
);
|
||||
|
||||
cleanup_user(&pool, shared_id).await;
|
||||
cleanup_user(&pool, alice_id).await;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user