mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
 Henry Park
|
060ce8de25 | ||
|
|
485d1568c4 | ||
|
|
acb590214a |
@@ -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.
|
||||
@@ -54,7 +54,7 @@ jobs:
|
||||
- group: features
|
||||
files: "tests/e2e/scenarios/test_skills.py tests/e2e/scenarios/test_tool_approval.py tests/e2e/scenarios/test_webhook.py"
|
||||
- group: extensions
|
||||
files: "tests/e2e/scenarios/test_extensions.py tests/e2e/scenarios/test_extension_oauth.py tests/e2e/scenarios/test_telegram_token_validation.py tests/e2e/scenarios/test_telegram_hot_activation.py tests/e2e/scenarios/test_wasm_lifecycle.py tests/e2e/scenarios/test_tool_execution.py tests/e2e/scenarios/test_pairing.py tests/e2e/scenarios/test_mcp_auth_flow.py tests/e2e/scenarios/test_oauth_credential_fallback.py tests/e2e/scenarios/test_routine_oauth_credential_injection.py"
|
||||
files: "tests/e2e/scenarios/test_extensions.py tests/e2e/scenarios/test_extension_oauth.py tests/e2e/scenarios/test_oauth_url_parameters.py tests/e2e/scenarios/test_telegram_token_validation.py tests/e2e/scenarios/test_telegram_hot_activation.py tests/e2e/scenarios/test_wasm_lifecycle.py tests/e2e/scenarios/test_tool_execution.py tests/e2e/scenarios/test_pairing.py tests/e2e/scenarios/test_mcp_auth_flow.py tests/e2e/scenarios/test_oauth_credential_fallback.py tests/e2e/scenarios/test_routine_oauth_credential_injection.py"
|
||||
- group: routines
|
||||
files: "tests/e2e/scenarios/test_owner_scope.py tests/e2e/scenarios/test_routine_event_batch.py"
|
||||
steps:
|
||||
|
||||
@@ -1,4 +0,0 @@
|
||||
{
|
||||
"setup": [],
|
||||
"teardown": []
|
||||
}
|
||||
Generated
+12
-12
@@ -157,7 +157,7 @@ version = "1.1.5"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "40c48f72fd53cd289104fc64099abca73db4166ad86ea0b4341abe65af83dadc"
|
||||
dependencies = [
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.60.2",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -168,7 +168,7 @@ checksum = "291e6a250ff86cd4a820112fb8898808a366d8f9f58ce16d1f538353ad55747d"
|
||||
dependencies = [
|
||||
"anstyle",
|
||||
"once_cell_polyfill",
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.60.2",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -2136,7 +2136,7 @@ dependencies = [
|
||||
"libc",
|
||||
"option-ext",
|
||||
"redox_users 0.5.2",
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.59.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -2323,7 +2323,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb"
|
||||
dependencies = [
|
||||
"libc",
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.52.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -4134,7 +4134,7 @@ version = "0.50.3"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5"
|
||||
dependencies = [
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.59.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -5472,7 +5472,7 @@ dependencies = [
|
||||
"errno",
|
||||
"libc",
|
||||
"linux-raw-sys 0.12.1",
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.52.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -6154,7 +6154,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "3a766e1110788c36f4fa1c2b71b387a7815aa65f88ce0229841826633d93723e"
|
||||
dependencies = [
|
||||
"libc",
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.60.2",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -6354,9 +6354,9 @@ checksum = "55937e1799185b12863d447f42597ed69d9928686b8d88a1df17376a097d8369"
|
||||
|
||||
[[package]]
|
||||
name = "tar"
|
||||
version = "0.4.44"
|
||||
version = "0.4.45"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "1d863878d212c87a19c1a610eb53bb01fe12951c0501cf5a0d65f724914a667a"
|
||||
checksum = "22692a6476a21fa75fdfc11d452fda482af402c008cdbaf3476414e122040973"
|
||||
dependencies = [
|
||||
"filetime",
|
||||
"libc",
|
||||
@@ -6379,7 +6379,7 @@ dependencies = [
|
||||
"getrandom 0.4.2",
|
||||
"once_cell",
|
||||
"rustix 1.1.4",
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.52.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -7179,7 +7179,7 @@ checksum = "f2f6fb2847f6742cd76af783a2a2c49e9375d0a111c7bef6f71cd9e738c72d6e"
|
||||
dependencies = [
|
||||
"memoffset",
|
||||
"tempfile",
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.60.2",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -8029,7 +8029,7 @@ version = "0.1.11"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22"
|
||||
dependencies = [
|
||||
"windows-sys 0.61.2",
|
||||
"windows-sys 0.48.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
|
||||
+1
-1
@@ -161,7 +161,7 @@ This document tracks feature parity between IronClaw (Rust implementation) and O
|
||||
| `config` | ✅ | ✅ | - | Read/write config plus validate/path helpers |
|
||||
| `backup` | ✅ | ❌ | P3 | Create/verify local backup archives |
|
||||
| `channels` | ✅ | 🚧 | P2 | `list` implemented; `enable`/`disable`/`status` deferred pending config source unification |
|
||||
| `models` | ✅ | 🚧 | - | Model selector in TUI |
|
||||
| `models` | ✅ | 🚧 | P1 | `models list [<provider>]` (`--verbose`, `--json`; fetches live model list when provider specified), `models status` (`--json`), `models set <model>`, `models set-provider <provider> [--model model]` (alias normalization, config.toml + .env persistence). Remaining: `set` doesn't validate model against live list. |
|
||||
| `status` | ✅ | ✅ | - | System status (enriched session details) |
|
||||
| `agents` | ✅ | ❌ | P3 | Multi-agent management |
|
||||
| `sessions` | ✅ | ❌ | P3 | Session listing (shows subagent models) |
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
+14
-2
@@ -131,6 +131,18 @@ impl Agent {
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(extension_manager) = self.deps.extension_manager.as_ref() {
|
||||
match extension_manager.llm_extension_state_summary().await {
|
||||
Ok(Some(summary)) => {
|
||||
reasoning = reasoning.with_extension_state_summary(summary);
|
||||
}
|
||||
Ok(None) => {}
|
||||
Err(e) => {
|
||||
tracing::debug!("Could not load extension state summary: {}", e);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(prompt) = system_prompt {
|
||||
reasoning = reasoning.with_system_prompt(prompt);
|
||||
}
|
||||
@@ -2140,7 +2152,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 +2257,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);
|
||||
}
|
||||
}
|
||||
|
||||
+7
-31
@@ -141,8 +141,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);
|
||||
}
|
||||
@@ -955,29 +954,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;
|
||||
@@ -1768,7 +1744,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 +2035,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 +2103,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 +2129,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 +2159,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 +2186,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);
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -25,6 +25,7 @@ pub mod import;
|
||||
mod logs;
|
||||
mod mcp;
|
||||
pub mod memory;
|
||||
mod models;
|
||||
pub mod oauth_defaults;
|
||||
mod pairing;
|
||||
mod registry;
|
||||
@@ -45,6 +46,7 @@ pub use logs::{LogsCommand, run_logs_command};
|
||||
pub use mcp::{McpCommand, run_mcp_command};
|
||||
pub use memory::MemoryCommand;
|
||||
pub use memory::run_memory_command_with_db;
|
||||
pub use models::{ModelsCommand, run_models_command};
|
||||
pub use pairing::{PairingCommand, run_pairing_command, run_pairing_command_with_store};
|
||||
pub use registry::{RegistryCommand, run_registry_command};
|
||||
pub use routines::{RoutinesCommand, run_routines_command};
|
||||
@@ -217,6 +219,14 @@ pub enum Command {
|
||||
)]
|
||||
Hooks(HooksCommand),
|
||||
|
||||
/// Manage LLM providers and models
|
||||
#[command(
|
||||
subcommand,
|
||||
about = "Manage LLM providers and models",
|
||||
long_about = "List providers, view current configuration, and set active provider/model.\nExamples:\n ironclaw models list\n ironclaw models list openai --verbose\n ironclaw models status\n ironclaw models set gpt-4o\n ironclaw models set-provider anthropic --model claude-sonnet-4-6-20250514"
|
||||
)]
|
||||
Models(ModelsCommand),
|
||||
|
||||
/// Probe external dependencies and validate configuration
|
||||
#[command(
|
||||
about = "Run diagnostics",
|
||||
|
||||
@@ -0,0 +1,864 @@
|
||||
//! Models management CLI commands.
|
||||
//!
|
||||
//! Provides subcommands for listing providers, viewing current model
|
||||
//! configuration, and setting the active provider/model. Settings are
|
||||
//! persisted to both `config.toml` and `~/.ironclaw/.env` so changes
|
||||
//! take effect immediately (no DB connection required).
|
||||
|
||||
use clap::Subcommand;
|
||||
use std::path::Path;
|
||||
|
||||
use crate::llm::registry::ProviderRegistry;
|
||||
use crate::settings::Settings;
|
||||
|
||||
#[derive(Subcommand, Debug, Clone)]
|
||||
pub enum ModelsCommand {
|
||||
/// List providers (or available models for a specific provider)
|
||||
List {
|
||||
/// Show only a specific provider (by ID or alias)
|
||||
provider: Option<String>,
|
||||
|
||||
/// Show detailed information (env vars, base URL, protocol)
|
||||
#[arg(short, long)]
|
||||
verbose: bool,
|
||||
|
||||
/// Output as JSON
|
||||
#[arg(long)]
|
||||
json: bool,
|
||||
},
|
||||
|
||||
/// Show current model configuration
|
||||
Status {
|
||||
/// Output as JSON
|
||||
#[arg(long)]
|
||||
json: bool,
|
||||
},
|
||||
|
||||
/// Set the default model
|
||||
Set {
|
||||
/// Model name (e.g., "gpt-5-mini", "claude-sonnet-4-6-20250514")
|
||||
model: String,
|
||||
},
|
||||
|
||||
/// Set the LLM provider
|
||||
SetProvider {
|
||||
/// Provider ID or alias (e.g., "openai", "anthropic", "ollama")
|
||||
provider: String,
|
||||
|
||||
/// Also set the model (defaults to provider's default model)
|
||||
#[arg(long)]
|
||||
model: Option<String>,
|
||||
},
|
||||
}
|
||||
|
||||
/// Run the models CLI subcommand.
|
||||
pub async fn run_models_command(
|
||||
cmd: ModelsCommand,
|
||||
config_path: Option<&Path>,
|
||||
) -> anyhow::Result<()> {
|
||||
match cmd {
|
||||
ModelsCommand::List {
|
||||
provider,
|
||||
verbose,
|
||||
json,
|
||||
} => {
|
||||
if let Some(ref id) = provider {
|
||||
cmd_show_provider(id, verbose, json, config_path).await
|
||||
} else {
|
||||
cmd_list_providers(verbose, json, config_path).await
|
||||
}
|
||||
}
|
||||
ModelsCommand::Status { json } => cmd_status(json, config_path),
|
||||
ModelsCommand::Set { model } => cmd_set_model(&model, config_path),
|
||||
ModelsCommand::SetProvider { provider, model } => {
|
||||
cmd_set_provider(&provider, model.as_deref(), config_path)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ─── Shared helpers ───────────────────────────────────────────────
|
||||
|
||||
/// Resolve the currently active backend and model from env + settings.
|
||||
fn resolve_active(config_path: Option<&Path>) -> (String, String) {
|
||||
let settings = load_settings(config_path);
|
||||
resolve_active_from_settings(&settings)
|
||||
}
|
||||
|
||||
/// Resolve active backend + model from a pre-loaded Settings.
|
||||
fn resolve_active_from_settings(settings: &Settings) -> (String, String) {
|
||||
let backend = std::env::var("LLM_BACKEND")
|
||||
.ok()
|
||||
.or_else(|| settings.llm_backend.clone())
|
||||
.unwrap_or_else(|| "nearai".to_string());
|
||||
|
||||
let registry = ProviderRegistry::load();
|
||||
|
||||
let canonical_backend = registry
|
||||
.find(&backend)
|
||||
.map(|d| d.id.clone())
|
||||
.unwrap_or_else(|| backend.clone());
|
||||
|
||||
let model = if canonical_backend == "nearai" {
|
||||
std::env::var("NEARAI_MODEL")
|
||||
.ok()
|
||||
.or_else(|| settings.selected_model.clone())
|
||||
.unwrap_or_else(|| "qwen2.5-72b-instruct:free".to_string())
|
||||
} else if let Some(def) = registry.find(&canonical_backend) {
|
||||
std::env::var(&def.model_env)
|
||||
.ok()
|
||||
.or_else(|| settings.selected_model.clone())
|
||||
.unwrap_or_else(|| def.default_model.clone())
|
||||
} else {
|
||||
settings
|
||||
.selected_model
|
||||
.clone()
|
||||
.unwrap_or_else(|| "unknown".to_string())
|
||||
};
|
||||
|
||||
(canonical_backend, model)
|
||||
}
|
||||
|
||||
fn load_settings(config_path: Option<&Path>) -> Settings {
|
||||
if let Some(path) = config_path {
|
||||
Settings::load_toml(path).ok().flatten().unwrap_or_default()
|
||||
} else {
|
||||
let toml_path = config_toml_path();
|
||||
if toml_path.exists() {
|
||||
Settings::load_toml(&toml_path)
|
||||
.ok()
|
||||
.flatten()
|
||||
.unwrap_or_default()
|
||||
} else {
|
||||
Settings::load()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn save_settings(settings: &Settings, config_path: Option<&Path>) -> anyhow::Result<()> {
|
||||
let path = config_path
|
||||
.map(|p| p.to_path_buf())
|
||||
.unwrap_or_else(config_toml_path);
|
||||
|
||||
settings
|
||||
.save_toml(&path)
|
||||
.map_err(|e| anyhow::anyhow!("{}", e))?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn config_toml_path() -> std::path::PathBuf {
|
||||
crate::bootstrap::ironclaw_base_dir().join("config.toml")
|
||||
}
|
||||
|
||||
/// Try to fetch the live model list from a provider.
|
||||
///
|
||||
/// Best-effort: returns `None` if config loading, provider creation, or the
|
||||
/// `list_models()` call fails (missing API key, network error, etc.).
|
||||
async fn try_fetch_models(provider_id: &str, config_path: Option<&Path>) -> Option<Vec<String>> {
|
||||
let config = crate::config::Config::from_env_with_toml(config_path)
|
||||
.await
|
||||
.ok()?;
|
||||
|
||||
// Override backend to the requested provider so create_llm_provider
|
||||
// constructs the right one.
|
||||
let mut llm_config = config.llm.clone();
|
||||
llm_config.backend = provider_id.to_string();
|
||||
|
||||
// For registry providers, resolve the RegistryProviderConfig if not
|
||||
// already set for this backend.
|
||||
if provider_id != "nearai" && provider_id != "bedrock" {
|
||||
let registry = ProviderRegistry::load();
|
||||
if let Some(def) = registry.find(provider_id)
|
||||
&& llm_config
|
||||
.provider
|
||||
.as_ref()
|
||||
.is_none_or(|p| p.provider_id != def.id)
|
||||
{
|
||||
// Build a minimal RegistryProviderConfig from env + registry
|
||||
let api_key = def
|
||||
.api_key_env
|
||||
.as_ref()
|
||||
.and_then(|env| std::env::var(env).ok());
|
||||
if def.api_key_required && api_key.is_none() {
|
||||
return None;
|
||||
}
|
||||
let base_url = def.default_base_url.clone().unwrap_or_default();
|
||||
llm_config.provider = Some(crate::llm::RegistryProviderConfig {
|
||||
protocol: def.protocol,
|
||||
provider_id: def.id.clone(),
|
||||
model: def.default_model.clone(),
|
||||
api_key: api_key.map(secrecy::SecretString::from),
|
||||
base_url,
|
||||
extra_headers: Vec::new(),
|
||||
oauth_token: None,
|
||||
is_codex_chatgpt: false,
|
||||
refresh_token: None,
|
||||
auth_path: None,
|
||||
cache_retention: Default::default(),
|
||||
unsupported_params: def.unsupported_params.clone(),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
let session = crate::llm::create_session_manager(config.llm.session.clone()).await;
|
||||
let provider = crate::llm::create_llm_provider(&llm_config, session)
|
||||
.await
|
||||
.ok()?;
|
||||
provider.list_models().await.ok().filter(|m| !m.is_empty())
|
||||
}
|
||||
|
||||
/// Print available models section (text output).
|
||||
fn print_model_list(models: &Option<Vec<String>>, active_model: Option<&String>) {
|
||||
match models {
|
||||
Some(models) => {
|
||||
println!("\n Available models ({}):", models.len());
|
||||
for m in models {
|
||||
let marker = active_model
|
||||
.filter(|a| a.as_str() == m)
|
||||
.map(|_| " (active)")
|
||||
.unwrap_or("");
|
||||
println!(" {}{}", m, marker);
|
||||
}
|
||||
}
|
||||
None => {
|
||||
println!(
|
||||
"\n Could not fetch model list (missing credentials or provider unavailable)."
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Also update `~/.ironclaw/.env` so changes take effect immediately.
|
||||
///
|
||||
/// Skipped when `config_path` is `Some` (custom `--config`), because the user
|
||||
/// is explicitly targeting a different config file and we must not pollute the
|
||||
/// default profile's `.env`.
|
||||
fn sync_to_dotenv(config_path: Option<&Path>, vars: &[(&str, &str)]) {
|
||||
if config_path.is_some() {
|
||||
return;
|
||||
}
|
||||
if let Err(e) = crate::bootstrap::upsert_bootstrap_vars(vars) {
|
||||
eprintln!("Warning: failed to update .env: {}", e);
|
||||
}
|
||||
}
|
||||
|
||||
// ─── status ───────────────────────────────────────────────────────
|
||||
|
||||
fn cmd_status(json: bool, config_path: Option<&Path>) -> anyhow::Result<()> {
|
||||
let settings = load_settings(config_path);
|
||||
let (backend, model) = resolve_active_from_settings(&settings);
|
||||
let registry = ProviderRegistry::load();
|
||||
|
||||
let fallback = std::env::var("NEARAI_FALLBACK_MODEL").ok();
|
||||
let cheap = std::env::var("NEARAI_CHEAP_MODEL").ok();
|
||||
|
||||
let description = if backend == "nearai" {
|
||||
"NEAR AI inference (default)".to_string()
|
||||
} else {
|
||||
registry
|
||||
.find(&backend)
|
||||
.map(|d| d.description.clone())
|
||||
.unwrap_or_default()
|
||||
};
|
||||
|
||||
if json {
|
||||
let v = serde_json::json!({
|
||||
"provider": backend,
|
||||
"model": model,
|
||||
"description": description,
|
||||
"fallback_model": fallback,
|
||||
"cheap_model": cheap,
|
||||
});
|
||||
println!(
|
||||
"{}",
|
||||
serde_json::to_string_pretty(&v).unwrap_or_else(|_| "{}".to_string())
|
||||
);
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
println!("Provider: {} ({})", backend, description);
|
||||
println!("Model: {}", model);
|
||||
if let Some(ref fb) = fallback {
|
||||
println!("Fallback: {}", fb);
|
||||
}
|
||||
if let Some(ref ch) = cheap {
|
||||
println!("Cheap: {}", ch);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// ─── set ──────────────────────────────────────────────────────────
|
||||
|
||||
fn cmd_set_model(model: &str, config_path: Option<&Path>) -> anyhow::Result<()> {
|
||||
let trimmed = model.trim();
|
||||
if trimmed.is_empty() {
|
||||
anyhow::bail!("Model name cannot be empty");
|
||||
}
|
||||
|
||||
let mut settings = load_settings(config_path);
|
||||
let registry = ProviderRegistry::load();
|
||||
|
||||
// Warn if model name doesn't match any known provider's default model
|
||||
let known_model = registry.all().iter().any(|d| d.default_model == trimmed)
|
||||
|| trimmed.contains("qwen") // nearai models
|
||||
|| trimmed.contains("llama")
|
||||
|| trimmed.contains("gpt")
|
||||
|| trimmed.contains("claude")
|
||||
|| trimmed.contains("gemini")
|
||||
|| trimmed.contains("mistral");
|
||||
if !known_model {
|
||||
eprintln!(
|
||||
"Warning: '{}' is not a recognized model name. Proceeding anyway.",
|
||||
trimmed
|
||||
);
|
||||
}
|
||||
|
||||
settings.selected_model = Some(trimmed.to_string());
|
||||
save_settings(&settings, config_path)?;
|
||||
|
||||
let backend = std::env::var("LLM_BACKEND")
|
||||
.ok()
|
||||
.or_else(|| settings.llm_backend.clone())
|
||||
.unwrap_or_else(|| "nearai".to_string());
|
||||
|
||||
// Also write to .env so the change takes effect immediately
|
||||
let model_env = if backend == "nearai" {
|
||||
"NEARAI_MODEL".to_string()
|
||||
} else {
|
||||
registry
|
||||
.find(&backend)
|
||||
.map(|d| d.model_env.clone())
|
||||
.unwrap_or_default()
|
||||
};
|
||||
if !model_env.is_empty() {
|
||||
sync_to_dotenv(config_path, &[(&model_env, trimmed)]);
|
||||
}
|
||||
|
||||
println!("Model set to '{}' (provider: {})", trimmed, backend);
|
||||
println!(
|
||||
"Saved to {}",
|
||||
config_path
|
||||
.map(|p| p.display().to_string())
|
||||
.unwrap_or_else(|| config_toml_path().display().to_string())
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// ─── set-provider ─────────────────────────────────────────────────
|
||||
|
||||
fn cmd_set_provider(
|
||||
provider: &str,
|
||||
model: Option<&str>,
|
||||
config_path: Option<&Path>,
|
||||
) -> anyhow::Result<()> {
|
||||
let registry = ProviderRegistry::load();
|
||||
|
||||
// Validate and normalize provider
|
||||
let canonical_id = if provider == "nearai" || provider == "near_ai" || provider == "near" {
|
||||
"nearai".to_string()
|
||||
} else {
|
||||
let def = registry.find(provider).ok_or_else(|| {
|
||||
let known: Vec<&str> = std::iter::once("nearai")
|
||||
.chain(registry.all().iter().map(|d| d.id.as_str()))
|
||||
.collect();
|
||||
anyhow::anyhow!(
|
||||
"Unknown provider '{}'. Known providers: {}",
|
||||
provider,
|
||||
known.join(", ")
|
||||
)
|
||||
})?;
|
||||
def.id.clone()
|
||||
};
|
||||
|
||||
// Resolve model: explicit > provider default
|
||||
let resolved_model = if let Some(m) = model {
|
||||
m.to_string()
|
||||
} else if canonical_id == "nearai" {
|
||||
"qwen2.5-72b-instruct:free".to_string()
|
||||
} else if let Some(def) = registry.find(&canonical_id) {
|
||||
def.default_model.clone()
|
||||
} else {
|
||||
"default".to_string()
|
||||
};
|
||||
|
||||
let mut settings = load_settings(config_path);
|
||||
settings.llm_backend = Some(canonical_id.clone());
|
||||
settings.selected_model = Some(resolved_model.clone());
|
||||
save_settings(&settings, config_path)?;
|
||||
|
||||
// Also write to .env so the change takes effect immediately
|
||||
let model_env = if canonical_id == "nearai" {
|
||||
"NEARAI_MODEL".to_string()
|
||||
} else {
|
||||
registry
|
||||
.find(&canonical_id)
|
||||
.map(|d| d.model_env.clone())
|
||||
.unwrap_or_default()
|
||||
};
|
||||
let mut vars: Vec<(&str, &str)> = vec![("LLM_BACKEND", &canonical_id)];
|
||||
if !model_env.is_empty() {
|
||||
vars.push((&model_env, &resolved_model));
|
||||
}
|
||||
sync_to_dotenv(config_path, &vars);
|
||||
|
||||
println!(
|
||||
"Provider set to '{}', model set to '{}'",
|
||||
canonical_id, resolved_model
|
||||
);
|
||||
println!(
|
||||
"Saved to {}",
|
||||
config_path
|
||||
.map(|p| p.display().to_string())
|
||||
.unwrap_or_else(|| config_toml_path().display().to_string())
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// ─── list ─────────────────────────────────────────────────────────
|
||||
|
||||
/// List all providers with their default models.
|
||||
async fn cmd_list_providers(
|
||||
verbose: bool,
|
||||
json: bool,
|
||||
config_path: Option<&Path>,
|
||||
) -> anyhow::Result<()> {
|
||||
let registry = ProviderRegistry::load();
|
||||
let (active_backend, active_model) = resolve_active(config_path);
|
||||
|
||||
if json {
|
||||
let mut entries: Vec<serde_json::Value> = Vec::new();
|
||||
|
||||
// NEAR AI (not in registry)
|
||||
let nearai_active = active_backend == "nearai";
|
||||
entries.push(serde_json::json!({
|
||||
"id": "nearai",
|
||||
"description": "NEAR AI inference (default)",
|
||||
"default_model": "qwen2.5-72b-instruct:free",
|
||||
"active": nearai_active,
|
||||
"active_model": if nearai_active { Some(&active_model) } else { None },
|
||||
}));
|
||||
|
||||
for def in registry.all() {
|
||||
let is_active = active_backend == def.id;
|
||||
let mut v = serde_json::json!({
|
||||
"id": def.id,
|
||||
"description": def.description,
|
||||
"default_model": def.default_model,
|
||||
"protocol": format!("{:?}", def.protocol),
|
||||
"active": is_active,
|
||||
});
|
||||
if is_active {
|
||||
v["active_model"] = serde_json::json!(active_model);
|
||||
}
|
||||
if verbose {
|
||||
v["aliases"] = serde_json::json!(def.aliases);
|
||||
v["model_env"] = serde_json::json!(def.model_env);
|
||||
v["api_key_env"] = serde_json::json!(def.api_key_env);
|
||||
v["api_key_required"] = serde_json::json!(def.api_key_required);
|
||||
if let Some(ref url) = def.default_base_url {
|
||||
v["base_url"] = serde_json::json!(url);
|
||||
}
|
||||
if let Some(ref setup) = def.setup {
|
||||
v["can_list_models"] = serde_json::json!(setup.can_list_models());
|
||||
}
|
||||
}
|
||||
entries.push(v);
|
||||
}
|
||||
|
||||
println!(
|
||||
"{}",
|
||||
serde_json::to_string_pretty(&entries).unwrap_or_else(|_| "[]".to_string())
|
||||
);
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let providers = registry.all();
|
||||
|
||||
println!("Active: {} (model: {})\n", active_backend, active_model);
|
||||
println!(
|
||||
"{} provider(s) available:\n",
|
||||
providers.len() + 1 // +1 for NEAR AI
|
||||
);
|
||||
|
||||
// NEAR AI (not in registry)
|
||||
let nearai_marker = if active_backend == "nearai" { " *" } else { "" };
|
||||
if verbose {
|
||||
println!(" nearai{}", nearai_marker);
|
||||
println!(" Description: NEAR AI inference (default)");
|
||||
println!(" Default model: qwen2.5-72b-instruct:free");
|
||||
println!(" Model env: NEARAI_MODEL");
|
||||
if active_backend == "nearai" {
|
||||
println!(" Active model: {}", active_model);
|
||||
}
|
||||
println!();
|
||||
} else {
|
||||
println!(
|
||||
" {:<22} {:<40} NEAR AI inference (default)",
|
||||
format!("nearai{nearai_marker}"),
|
||||
"qwen2.5-72b-instruct:free"
|
||||
);
|
||||
}
|
||||
|
||||
for def in providers {
|
||||
let is_active = active_backend == def.id;
|
||||
let marker = if is_active { " *" } else { "" };
|
||||
|
||||
if verbose {
|
||||
println!(" {}{}", def.id, marker);
|
||||
println!(" Description: {}", def.description);
|
||||
println!(" Default model: {}", def.default_model);
|
||||
println!(" Protocol: {:?}", def.protocol);
|
||||
println!(" Model env: {}", def.model_env);
|
||||
if let Some(ref env) = def.api_key_env {
|
||||
println!(
|
||||
" API key env: {} ({})",
|
||||
env,
|
||||
if def.api_key_required {
|
||||
"required"
|
||||
} else {
|
||||
"optional"
|
||||
}
|
||||
);
|
||||
}
|
||||
if let Some(ref url) = def.default_base_url {
|
||||
println!(" Base URL: {}", url);
|
||||
}
|
||||
if !def.aliases.is_empty() {
|
||||
println!(" Aliases: {}", def.aliases.join(", "));
|
||||
}
|
||||
if is_active {
|
||||
println!(" Active model: {}", active_model);
|
||||
}
|
||||
println!();
|
||||
} else {
|
||||
let model_display = if is_active {
|
||||
active_model.clone()
|
||||
} else {
|
||||
def.default_model.clone()
|
||||
};
|
||||
println!(
|
||||
" {:<22} {:<40} {}",
|
||||
format!("{}{marker}", def.id),
|
||||
model_display,
|
||||
def.description,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
if !verbose {
|
||||
println!();
|
||||
println!("* = active provider. Use --verbose for details.");
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Show details for a specific provider.
|
||||
async fn cmd_show_provider(
|
||||
id: &str,
|
||||
verbose: bool,
|
||||
json: bool,
|
||||
config_path: Option<&Path>,
|
||||
) -> anyhow::Result<()> {
|
||||
let registry = ProviderRegistry::load();
|
||||
let (active_backend, active_model) = resolve_active(config_path);
|
||||
|
||||
// Resolve canonical ID for model fetching
|
||||
let canonical_id = if id == "nearai" || id == "near_ai" || id == "near" {
|
||||
"nearai".to_string()
|
||||
} else {
|
||||
registry
|
||||
.find(id)
|
||||
.map(|d| d.id.clone())
|
||||
.unwrap_or_else(|| id.to_string())
|
||||
};
|
||||
|
||||
// Try to fetch live model list from the provider
|
||||
let live_models = try_fetch_models(&canonical_id, config_path).await;
|
||||
|
||||
// Check NEAR AI first (not in registry)
|
||||
if id == "nearai" || id == "near_ai" || id == "near" {
|
||||
let is_active = active_backend == "nearai";
|
||||
if json {
|
||||
let mut v = serde_json::json!({
|
||||
"id": "nearai",
|
||||
"description": "NEAR AI inference (default)",
|
||||
"default_model": "qwen2.5-72b-instruct:free",
|
||||
"model_env": "NEARAI_MODEL",
|
||||
"active": is_active,
|
||||
});
|
||||
if is_active {
|
||||
v["active_model"] = serde_json::json!(active_model);
|
||||
}
|
||||
if let Some(ref models) = live_models {
|
||||
v["available_models"] = serde_json::json!(models);
|
||||
}
|
||||
println!(
|
||||
"{}",
|
||||
serde_json::to_string_pretty(&v).unwrap_or_else(|_| "{}".to_string())
|
||||
);
|
||||
} else {
|
||||
println!("Provider: nearai");
|
||||
println!(" Description: NEAR AI inference (default)");
|
||||
println!(" Default model: qwen2.5-72b-instruct:free");
|
||||
println!(" Model env: NEARAI_MODEL");
|
||||
println!(" Active: {}", if is_active { "yes" } else { "no" });
|
||||
if is_active {
|
||||
println!(" Active model: {}", active_model);
|
||||
}
|
||||
print_model_list(&live_models, is_active.then_some(&active_model));
|
||||
}
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let def = registry.find(id).ok_or_else(|| {
|
||||
let known: Vec<&str> = std::iter::once("nearai")
|
||||
.chain(registry.all().iter().map(|d| d.id.as_str()))
|
||||
.collect();
|
||||
anyhow::anyhow!(
|
||||
"Unknown provider '{}'. Known providers: {}",
|
||||
id,
|
||||
known.join(", ")
|
||||
)
|
||||
})?;
|
||||
|
||||
let is_active = active_backend == def.id;
|
||||
|
||||
if json {
|
||||
let mut v = serde_json::json!({
|
||||
"id": def.id,
|
||||
"description": def.description,
|
||||
"protocol": format!("{:?}", def.protocol),
|
||||
"default_model": def.default_model,
|
||||
"model_env": def.model_env,
|
||||
"api_key_env": def.api_key_env,
|
||||
"api_key_required": def.api_key_required,
|
||||
"aliases": def.aliases,
|
||||
"active": is_active,
|
||||
});
|
||||
if let Some(ref url) = def.default_base_url {
|
||||
v["base_url"] = serde_json::json!(url);
|
||||
}
|
||||
if let Some(ref setup) = def.setup {
|
||||
v["can_list_models"] = serde_json::json!(setup.can_list_models());
|
||||
v["display_name"] = serde_json::json!(setup.display_name());
|
||||
}
|
||||
if is_active {
|
||||
v["active_model"] = serde_json::json!(active_model);
|
||||
}
|
||||
if verbose && !def.unsupported_params.is_empty() {
|
||||
v["unsupported_params"] = serde_json::json!(def.unsupported_params);
|
||||
}
|
||||
if let Some(ref models) = live_models {
|
||||
v["available_models"] = serde_json::json!(models);
|
||||
}
|
||||
println!(
|
||||
"{}",
|
||||
serde_json::to_string_pretty(&v).unwrap_or_else(|_| "{}".to_string())
|
||||
);
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
println!("Provider: {}", def.id);
|
||||
println!(" Description: {}", def.description);
|
||||
println!(" Protocol: {:?}", def.protocol);
|
||||
println!(" Default model: {}", def.default_model);
|
||||
println!(" Model env: {}", def.model_env);
|
||||
if let Some(ref env) = def.api_key_env {
|
||||
println!(
|
||||
" API key env: {} ({})",
|
||||
env,
|
||||
if def.api_key_required {
|
||||
"required"
|
||||
} else {
|
||||
"optional"
|
||||
}
|
||||
);
|
||||
}
|
||||
if let Some(ref url) = def.default_base_url {
|
||||
println!(" Base URL: {}", url);
|
||||
}
|
||||
if !def.aliases.is_empty() {
|
||||
println!(" Aliases: {}", def.aliases.join(", "));
|
||||
}
|
||||
if let Some(ref setup) = def.setup {
|
||||
println!(
|
||||
" List models: {}",
|
||||
if setup.can_list_models() {
|
||||
"supported"
|
||||
} else {
|
||||
"not supported"
|
||||
}
|
||||
);
|
||||
println!(" Display name: {}", setup.display_name());
|
||||
}
|
||||
if !def.unsupported_params.is_empty() {
|
||||
println!(" Unsupported: {}", def.unsupported_params.join(", "));
|
||||
}
|
||||
println!(" Active: {}", if is_active { "yes" } else { "no" });
|
||||
if is_active {
|
||||
println!(" Active model: {}", active_model);
|
||||
}
|
||||
print_model_list(&live_models, is_active.then_some(&active_model));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn resolve_active_defaults_to_nearai() {
|
||||
let settings = Settings::default();
|
||||
assert!(settings.llm_backend.is_none());
|
||||
assert!(settings.selected_model.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn registry_loads_all_providers() {
|
||||
let registry = ProviderRegistry::load();
|
||||
let all = registry.all();
|
||||
assert!(
|
||||
all.len() >= 10,
|
||||
"should have at least 10 built-in providers, got {}",
|
||||
all.len()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn registry_find_by_alias() {
|
||||
let registry = ProviderRegistry::load();
|
||||
let def = registry
|
||||
.find("claude")
|
||||
.expect("claude alias should resolve");
|
||||
assert_eq!(def.id, "anthropic");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn all_providers_have_description() {
|
||||
let registry = ProviderRegistry::load();
|
||||
for def in registry.all() {
|
||||
assert!(
|
||||
!def.description.is_empty(),
|
||||
"provider {} should have a description",
|
||||
def.id
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_model_persists_to_toml() {
|
||||
let dir = tempfile::tempdir().expect("create temp dir");
|
||||
let toml_path = dir.path().join("config.toml");
|
||||
|
||||
cmd_set_model("gpt-5-mini", Some(&toml_path)).expect("set model");
|
||||
|
||||
let settings = Settings::load_toml(&toml_path)
|
||||
.expect("read toml")
|
||||
.expect("should have settings");
|
||||
assert_eq!(settings.selected_model.as_deref(), Some("gpt-5-mini"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_provider_validates_unknown() {
|
||||
let dir = tempfile::tempdir().expect("create temp dir");
|
||||
let toml_path = dir.path().join("config.toml");
|
||||
|
||||
let result = cmd_set_provider("nonexistent_provider", None, Some(&toml_path));
|
||||
assert!(result.is_err());
|
||||
let err = result.unwrap_err().to_string();
|
||||
assert!(
|
||||
err.contains("Unknown provider"),
|
||||
"should mention unknown provider: {}",
|
||||
err
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_provider_persists_to_toml() {
|
||||
let dir = tempfile::tempdir().expect("create temp dir");
|
||||
let toml_path = dir.path().join("config.toml");
|
||||
|
||||
cmd_set_provider("groq", None, Some(&toml_path)).expect("set provider");
|
||||
|
||||
let settings = Settings::load_toml(&toml_path)
|
||||
.expect("read toml")
|
||||
.expect("should have settings");
|
||||
assert_eq!(settings.llm_backend.as_deref(), Some("groq"));
|
||||
assert_eq!(
|
||||
settings.selected_model.as_deref(),
|
||||
Some("llama-3.3-70b-versatile")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_provider_with_custom_model() {
|
||||
let dir = tempfile::tempdir().expect("create temp dir");
|
||||
let toml_path = dir.path().join("config.toml");
|
||||
|
||||
cmd_set_provider("anthropic", Some("claude-opus-4-6"), Some(&toml_path))
|
||||
.expect("set provider with model");
|
||||
|
||||
let settings = Settings::load_toml(&toml_path)
|
||||
.expect("read toml")
|
||||
.expect("should have settings");
|
||||
assert_eq!(settings.llm_backend.as_deref(), Some("anthropic"));
|
||||
assert_eq!(settings.selected_model.as_deref(), Some("claude-opus-4-6"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn custom_config_does_not_pollute_default_dotenv() {
|
||||
let dir = tempfile::tempdir().expect("create temp dir");
|
||||
let toml_path = dir.path().join("config.toml");
|
||||
|
||||
// With a custom config path, sync_to_dotenv should be a no-op
|
||||
// (it returns early when config_path is Some).
|
||||
// We verify by checking that cmd_set_provider succeeds without
|
||||
// trying to write to the default ~/.ironclaw/.env.
|
||||
cmd_set_provider("groq", None, Some(&toml_path)).expect("set provider with custom config");
|
||||
|
||||
let settings = Settings::load_toml(&toml_path)
|
||||
.expect("read toml")
|
||||
.expect("should have settings");
|
||||
assert_eq!(settings.llm_backend.as_deref(), Some("groq"));
|
||||
// The key assertion is that no error was thrown trying to write
|
||||
// to the default .env — sync_to_dotenv skipped it.
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_model_rejects_empty_name() {
|
||||
let dir = tempfile::tempdir().expect("create temp dir");
|
||||
let toml_path = dir.path().join("config.toml");
|
||||
|
||||
let result = cmd_set_model("", Some(&toml_path));
|
||||
assert!(result.is_err());
|
||||
assert!(
|
||||
result.unwrap_err().to_string().contains("cannot be empty"),
|
||||
"should reject empty model name"
|
||||
);
|
||||
|
||||
let result2 = cmd_set_model(" ", Some(&toml_path));
|
||||
assert!(result2.is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_provider_normalizes_alias() {
|
||||
let dir = tempfile::tempdir().expect("create temp dir");
|
||||
let toml_path = dir.path().join("config.toml");
|
||||
|
||||
cmd_set_provider("claude", None, Some(&toml_path)).expect("set via alias");
|
||||
|
||||
let settings = Settings::load_toml(&toml_path)
|
||||
.expect("read toml")
|
||||
.expect("should have settings");
|
||||
assert_eq!(
|
||||
settings.llm_backend.as_deref(),
|
||||
Some("anthropic"),
|
||||
"alias should be normalized to canonical ID"
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -20,6 +20,7 @@ Commands:
|
||||
service Manage OS service
|
||||
skills Manage skills
|
||||
hooks Manage lifecycle hooks
|
||||
models Manage LLM providers and models
|
||||
doctor Run diagnostics
|
||||
logs View and manage gateway logs
|
||||
status Show system status
|
||||
|
||||
@@ -20,6 +20,7 @@ Commands:
|
||||
service Manage OS service
|
||||
skills Manage skills
|
||||
hooks Manage lifecycle hooks
|
||||
models Manage LLM providers and models
|
||||
doctor Run diagnostics
|
||||
logs View and manage gateway logs
|
||||
status Show system status
|
||||
|
||||
@@ -23,6 +23,7 @@ Commands:
|
||||
service Manage OS service
|
||||
skills Manage skills
|
||||
hooks Manage lifecycle hooks
|
||||
models Manage LLM providers and models
|
||||
doctor Run diagnostics
|
||||
logs View and manage gateway logs
|
||||
status Show system status
|
||||
|
||||
@@ -23,6 +23,7 @@ Commands:
|
||||
service Manage OS service
|
||||
skills Manage skills
|
||||
hooks Manage lifecycle hooks
|
||||
models Manage LLM providers and models
|
||||
doctor Run diagnostics
|
||||
logs View and manage gateway logs
|
||||
status Show system status
|
||||
|
||||
@@ -1498,6 +1498,52 @@ impl ExtensionManager {
|
||||
Ok(extensions)
|
||||
}
|
||||
|
||||
/// Build a compact, deterministic extension snapshot for LLM prompt context.
|
||||
pub async fn llm_extension_state_summary(&self) -> Result<Option<String>, ExtensionError> {
|
||||
let mut extensions = self.list(None, false).await?;
|
||||
extensions.sort_by(|a, b| {
|
||||
llm_extension_sort_key(a.kind)
|
||||
.cmp(&llm_extension_sort_key(b.kind))
|
||||
.then_with(|| a.name.cmp(&b.name))
|
||||
});
|
||||
|
||||
let mut channels = Vec::new();
|
||||
let mut tools = Vec::new();
|
||||
let mut servers = Vec::new();
|
||||
|
||||
for extension in extensions {
|
||||
let owner_bound = matches!(extension.kind, ExtensionKind::WasmChannel)
|
||||
&& self.has_wasm_channel_owner_binding(&extension.name).await;
|
||||
if !(extension.active || extension.authenticated || owner_bound) {
|
||||
continue;
|
||||
}
|
||||
|
||||
let item = llm_extension_summary_item(&extension, owner_bound);
|
||||
match extension.kind {
|
||||
ExtensionKind::WasmChannel | ExtensionKind::ChannelRelay => channels.push(item),
|
||||
ExtensionKind::WasmTool => tools.push(item),
|
||||
ExtensionKind::McpServer => servers.push(item),
|
||||
}
|
||||
}
|
||||
|
||||
let mut lines = Vec::new();
|
||||
if !channels.is_empty() {
|
||||
lines.push(format!("- Channels: {}", channels.join("; ")));
|
||||
}
|
||||
if !tools.is_empty() {
|
||||
lines.push(format!("- Tools: {}", tools.join("; ")));
|
||||
}
|
||||
if !servers.is_empty() {
|
||||
lines.push(format!("- MCP servers: {}", servers.join("; ")));
|
||||
}
|
||||
|
||||
if lines.is_empty() {
|
||||
Ok(None)
|
||||
} else {
|
||||
Ok(Some(lines.join("\n")))
|
||||
}
|
||||
}
|
||||
|
||||
/// Remove an installed extension.
|
||||
pub async fn remove(&self, name: &str) -> Result<String, ExtensionError> {
|
||||
Self::validate_extension_name(name)?;
|
||||
@@ -5613,6 +5659,40 @@ fn combine_install_errors(
|
||||
}
|
||||
}
|
||||
|
||||
fn llm_extension_sort_key(kind: ExtensionKind) -> u8 {
|
||||
match kind {
|
||||
ExtensionKind::WasmChannel | ExtensionKind::ChannelRelay => 0,
|
||||
ExtensionKind::WasmTool => 1,
|
||||
ExtensionKind::McpServer => 2,
|
||||
}
|
||||
}
|
||||
|
||||
fn llm_extension_summary_item(extension: &InstalledExtension, owner_bound: bool) -> String {
|
||||
let mut states = Vec::new();
|
||||
if extension.authenticated {
|
||||
states.push("authenticated".to_string());
|
||||
}
|
||||
if extension.active {
|
||||
states.push("active".to_string());
|
||||
} else if extension.authenticated {
|
||||
states.push("inactive".to_string());
|
||||
}
|
||||
if owner_bound {
|
||||
states.push("owner-bound".to_string());
|
||||
}
|
||||
if !extension.tools.is_empty() {
|
||||
let mut tool_names = extension.tools.clone();
|
||||
tool_names.sort();
|
||||
states.push(format!("tools: {}", tool_names.join(", ")));
|
||||
}
|
||||
|
||||
if states.is_empty() {
|
||||
extension.name.clone()
|
||||
} else {
|
||||
format!("{} ({})", extension.name, states.join(", "))
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use std::fmt::Debug;
|
||||
@@ -6571,6 +6651,142 @@ mod tests {
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(feature = "libsql")]
|
||||
#[tokio::test]
|
||||
async fn test_llm_extension_state_summary_reports_active_owner_bound_telegram()
|
||||
-> Result<(), String> {
|
||||
let dir = tempfile::tempdir().map_err(|err| format!("temp dir: {err}"))?;
|
||||
let channels_dir = dir.path().join("channels");
|
||||
std::fs::create_dir_all(&channels_dir).map_err(|err| format!("channels dir: {err}"))?;
|
||||
std::fs::write(channels_dir.join("telegram.wasm"), b"mock")
|
||||
.map_err(|err| format!("write wasm: {err}"))?;
|
||||
std::fs::write(
|
||||
channels_dir.join("telegram.capabilities.json"),
|
||||
serde_json::to_vec(&serde_json::json!({
|
||||
"type": "channel",
|
||||
"name": "telegram",
|
||||
"setup": {
|
||||
"required_secrets": [
|
||||
{
|
||||
"name": "telegram_bot_token",
|
||||
"prompt": "Enter your Telegram Bot API token (from @BotFather)",
|
||||
"optional": false
|
||||
}
|
||||
]
|
||||
},
|
||||
"capabilities": {
|
||||
"channel": {
|
||||
"allowed_paths": ["/webhook/telegram"]
|
||||
}
|
||||
},
|
||||
"config": {
|
||||
"owner_id": null
|
||||
}
|
||||
}))
|
||||
.map_err(|err| format!("serialize capabilities: {err}"))?,
|
||||
)
|
||||
.map_err(|err| format!("write capabilities: {err}"))?;
|
||||
|
||||
let (db, _db_tmp) = crate::testing::test_db().await;
|
||||
let manager = {
|
||||
use crate::secrets::{InMemorySecretsStore, SecretsCrypto};
|
||||
use crate::testing::credentials::TEST_CRYPTO_KEY;
|
||||
use crate::tools::ToolRegistry;
|
||||
use crate::tools::mcp::process::McpProcessManager;
|
||||
use crate::tools::mcp::session::McpSessionManager;
|
||||
|
||||
let master_key = secrecy::SecretString::from(TEST_CRYPTO_KEY.to_string());
|
||||
let crypto = Arc::new(
|
||||
SecretsCrypto::new(master_key)
|
||||
.map_err(|err| format!("failed to construct test crypto: {err}"))?,
|
||||
);
|
||||
|
||||
ExtensionManager::new(
|
||||
Arc::new(McpSessionManager::new()),
|
||||
Arc::new(McpProcessManager::new()),
|
||||
Arc::new(InMemorySecretsStore::new(crypto)),
|
||||
Arc::new(ToolRegistry::new()),
|
||||
None,
|
||||
None,
|
||||
dir.path().join("tools"),
|
||||
channels_dir.clone(),
|
||||
None,
|
||||
"test".to_string(),
|
||||
Some(db),
|
||||
Vec::new(),
|
||||
)
|
||||
};
|
||||
|
||||
let channel_manager = Arc::new(ChannelManager::new());
|
||||
let runtime = Arc::new(
|
||||
WasmChannelRuntime::new(WasmChannelRuntimeConfig::for_testing())
|
||||
.map_err(|err| format!("runtime: {err}"))?,
|
||||
);
|
||||
let pairing_store = Arc::new(PairingStore::with_base_dir(
|
||||
dir.path().join("pairing-state"),
|
||||
));
|
||||
let router = Arc::new(WasmChannelRouter::new());
|
||||
manager
|
||||
.set_channel_runtime(
|
||||
Arc::clone(&channel_manager),
|
||||
Arc::clone(&runtime),
|
||||
Arc::clone(&pairing_store),
|
||||
Arc::clone(&router),
|
||||
std::collections::HashMap::new(),
|
||||
)
|
||||
.await;
|
||||
manager
|
||||
.set_test_wasm_channel_loader(Arc::new({
|
||||
let runtime = Arc::clone(&runtime);
|
||||
let pairing_store = Arc::clone(&pairing_store);
|
||||
move |name| {
|
||||
Ok(make_test_loaded_channel(
|
||||
Arc::clone(&runtime),
|
||||
name,
|
||||
Arc::clone(&pairing_store),
|
||||
))
|
||||
}
|
||||
}))
|
||||
.await;
|
||||
manager
|
||||
.set_test_telegram_binding_resolver(Arc::new(|_token, existing_owner_id| {
|
||||
if existing_owner_id.is_some() {
|
||||
return Err(ExtensionError::Other(
|
||||
"owner binding should be derived during setup".to_string(),
|
||||
));
|
||||
}
|
||||
Ok(TelegramBindingResult::Bound(TelegramBindingData {
|
||||
owner_id: 424242,
|
||||
bot_username: Some("test_hot_bot".to_string()),
|
||||
binding_state: TelegramOwnerBindingState::VerifiedNow,
|
||||
}))
|
||||
}))
|
||||
.await;
|
||||
|
||||
manager
|
||||
.configure(
|
||||
"telegram",
|
||||
&std::collections::HashMap::from([(
|
||||
"telegram_bot_token".to_string(),
|
||||
"123456789:ABCdefGhI".to_string(),
|
||||
)]),
|
||||
&std::collections::HashMap::new(),
|
||||
)
|
||||
.await
|
||||
.map_err(|err| format!("configure succeeds: {err}"))?;
|
||||
|
||||
let summary = manager
|
||||
.llm_extension_state_summary()
|
||||
.await
|
||||
.map_err(|err| format!("summary: {err}"))?
|
||||
.ok_or_else(|| "expected extension summary".to_string())?;
|
||||
|
||||
require(
|
||||
summary.contains("- Channels: telegram (authenticated, active, owner-bound)"),
|
||||
format!("unexpected summary: {summary}"),
|
||||
)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_telegram_hot_activation_returns_verification_challenge_before_binding()
|
||||
-> Result<(), String> {
|
||||
|
||||
+92
-12
@@ -353,6 +353,8 @@ pub struct Reasoning {
|
||||
workspace_system_prompt: Option<String>,
|
||||
/// Optional skill context block to inject into system prompt.
|
||||
skill_context: Option<String>,
|
||||
/// Optional snapshot of connected/active extensions for the current user.
|
||||
extension_state_summary: Option<String>,
|
||||
/// Channel name (e.g. "discord", "telegram") for formatting hints.
|
||||
channel: Option<String>,
|
||||
/// Model name for runtime context.
|
||||
@@ -371,6 +373,7 @@ impl Reasoning {
|
||||
llm,
|
||||
workspace_system_prompt: None,
|
||||
skill_context: None,
|
||||
extension_state_summary: None,
|
||||
channel: None,
|
||||
model_name: None,
|
||||
is_group_chat: false,
|
||||
@@ -400,6 +403,14 @@ impl Reasoning {
|
||||
self
|
||||
}
|
||||
|
||||
/// Set extension runtime context to inject into the system prompt.
|
||||
pub fn with_extension_state_summary(mut self, summary: String) -> Self {
|
||||
if !summary.is_empty() {
|
||||
self.extension_state_summary = Some(summary);
|
||||
}
|
||||
self
|
||||
}
|
||||
|
||||
/// Set the channel name for channel-specific formatting hints.
|
||||
pub fn with_channel(mut self, channel: impl Into<String>) -> Self {
|
||||
let ch = channel.into();
|
||||
@@ -932,21 +943,54 @@ Example:
|
||||
}
|
||||
|
||||
fn build_extensions_section_for_tools(&self, tools: &[ToolDefinition]) -> String {
|
||||
// Only include when the extension management tools are available
|
||||
let has_ext_tools = tools.iter().any(|t| t.name == "tool_search");
|
||||
if !has_ext_tools {
|
||||
let has_search = tools.iter().any(|t| t.name == "tool_search");
|
||||
let has_list = tools.iter().any(|t| t.name == "tool_list");
|
||||
let has_info = tools.iter().any(|t| t.name == "extension_info");
|
||||
|
||||
if self.extension_state_summary.is_none() && !has_search && !has_list && !has_info {
|
||||
return String::new();
|
||||
}
|
||||
|
||||
"\n\n## Extensions\n\
|
||||
You can search, install, and activate extensions to add new capabilities:\n\
|
||||
- **Channels** (Telegram, Slack, Discord) — messaging integrations. \
|
||||
When users ask about connecting a messaging platform, search for it as a channel.\n\
|
||||
- **Tools** — sandboxed functions that extend your abilities.\n\
|
||||
- **MCP servers** — external API integrations via the Model Context Protocol.\n\n\
|
||||
Use `tool_search` to find extensions by name. Refer to them by their kind \
|
||||
(channel, tool, or server) — not as \"MCP server\" generically."
|
||||
.to_string()
|
||||
let mut blocks = Vec::new();
|
||||
if let Some(ref summary) = self.extension_state_summary {
|
||||
blocks.push(format!(
|
||||
"Current extension state for this user:\n{}",
|
||||
summary
|
||||
));
|
||||
}
|
||||
|
||||
if has_search || has_list || has_info {
|
||||
let mut guidance = String::from(
|
||||
"You can search, install, and activate extensions to add new capabilities:\n\
|
||||
- **Channels** (Telegram, Slack, Discord) — messaging integrations. \
|
||||
When users ask about connecting a messaging platform, search for it as a channel.\n\
|
||||
- **Tools** — sandboxed functions that extend your abilities.\n\
|
||||
- **MCP servers** — external API integrations via the Model Context Protocol.",
|
||||
);
|
||||
|
||||
if has_list {
|
||||
guidance.push_str(
|
||||
"\n\nBefore telling the user to connect, activate, or re-enable an extension, \
|
||||
inspect the current state with `tool_list`.",
|
||||
);
|
||||
}
|
||||
if has_info {
|
||||
guidance.push_str(
|
||||
"\nUse `extension_info` when you need deeper compatibility or runtime details \
|
||||
for an installed extension.",
|
||||
);
|
||||
}
|
||||
if has_search {
|
||||
guidance.push_str(
|
||||
"\nUse `tool_search` to find extensions by name. Refer to them by their kind \
|
||||
(channel, tool, or server) — not as \"MCP server\" generically.",
|
||||
);
|
||||
}
|
||||
|
||||
blocks.push(guidance);
|
||||
}
|
||||
|
||||
format!("\n\n## Extensions\n{}", blocks.join("\n\n"))
|
||||
}
|
||||
|
||||
fn build_channel_section(&self) -> String {
|
||||
@@ -2285,6 +2329,42 @@ That's my plan."#;
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_system_prompt_includes_extension_runtime_summary() {
|
||||
let reasoning = make_test_reasoning().with_extension_state_summary(
|
||||
"- Channels: telegram (authenticated, active, owner-bound)".to_string(),
|
||||
);
|
||||
|
||||
let prompt = reasoning.build_system_prompt_with_tools(&[]);
|
||||
assert!(
|
||||
prompt.contains("## Extensions"),
|
||||
"Prompt should contain an Extensions section when runtime state is present"
|
||||
);
|
||||
assert!(
|
||||
prompt.contains("telegram (authenticated, active, owner-bound)"),
|
||||
"Prompt should include the injected extension runtime summary"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_system_prompt_extension_guidance_prefers_inspection_before_reconnect() {
|
||||
let reasoning = make_test_reasoning();
|
||||
let prompt = reasoning.build_system_prompt_with_tools(&make_tools(&[
|
||||
"tool_search",
|
||||
"tool_list",
|
||||
"extension_info",
|
||||
]));
|
||||
|
||||
assert!(
|
||||
prompt.contains("inspect the current state with `tool_list`"),
|
||||
"Prompt should direct the model to inspect installed/active state first"
|
||||
);
|
||||
assert!(
|
||||
prompt.contains("Use `extension_info` when you need deeper compatibility"),
|
||||
"Prompt should mention extension_info for deeper extension details"
|
||||
);
|
||||
}
|
||||
|
||||
// ---- plan/evaluate bypass clean_response (Bug #564-2) ----
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -142,6 +142,11 @@ async fn async_main() -> anyhow::Result<()> {
|
||||
init_cli_tracing();
|
||||
return ironclaw::cli::run_logs_command(logs_cmd.clone(), cli.config.as_deref()).await;
|
||||
}
|
||||
Some(Command::Models(models_cmd)) => {
|
||||
init_cli_tracing();
|
||||
return ironclaw::cli::run_models_command(models_cmd.clone(), cli.config.as_deref())
|
||||
.await;
|
||||
}
|
||||
Some(Command::Doctor) => {
|
||||
init_cli_tracing();
|
||||
return ironclaw::cli::run_doctor_command().await;
|
||||
|
||||
@@ -0,0 +1,249 @@
|
||||
"""OAuth URL parameter validation e2e tests.
|
||||
|
||||
Tests for bug #992: Google OAuth URL broken when initiated from Telegram.
|
||||
Specifically verifies that OAuth query parameters are correctly formatted:
|
||||
- "client_id" (with underscore) NOT "clientid" (without underscore)
|
||||
- All standard OAuth parameters are present and correctly encoded
|
||||
- URLs are consistent across channels (web, Telegram, etc.)
|
||||
|
||||
The test verifies:
|
||||
1. OAuth URL is generated with correct parameters
|
||||
2. URL works with the OAuth provider (Google)
|
||||
3. Extra parameters (access_type, prompt) are preserved
|
||||
"""
|
||||
|
||||
from urllib.parse import parse_qs, urlparse
|
||||
import pytest
|
||||
|
||||
from helpers import api_post, api_get
|
||||
|
||||
|
||||
async def _extract_oauth_params(auth_url: str) -> dict:
|
||||
"""Extract and validate OAuth query parameters from auth_url.
|
||||
|
||||
Returns dict with parsed parameters:
|
||||
{
|
||||
'client_id': '...',
|
||||
'redirect_uri': '...',
|
||||
'response_type': 'code',
|
||||
'scope': '...',
|
||||
'state': '...',
|
||||
'access_type': '...',
|
||||
'prompt': '...',
|
||||
...
|
||||
}
|
||||
"""
|
||||
parsed = urlparse(auth_url)
|
||||
qs = parse_qs(parsed.query)
|
||||
|
||||
# Convert lists to single values for easier testing
|
||||
params = {k: v[0] if len(v) > 0 else v for k, v in qs.items()}
|
||||
return params
|
||||
|
||||
|
||||
async def _get_extension(ironclaw_server, name):
|
||||
"""Get a specific extension from the extensions list, or None."""
|
||||
r = await api_get(ironclaw_server, "/api/extensions")
|
||||
for ext in r.json().get("extensions", []):
|
||||
if ext["name"] == name:
|
||||
return ext
|
||||
return None
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
async def installed_gmail(ironclaw_server):
|
||||
"""Installs the 'gmail' extension before a test and removes it after.
|
||||
|
||||
This fixture handles the setup and teardown of the Gmail extension,
|
||||
ensuring a clean state for each test.
|
||||
"""
|
||||
# Ensure Gmail is not installed before test
|
||||
ext = await _get_extension(ironclaw_server, "gmail")
|
||||
if ext:
|
||||
r = await api_post(ironclaw_server, "/api/extensions/gmail/remove", timeout=30)
|
||||
assert r.status_code == 200
|
||||
|
||||
# Install Gmail
|
||||
r = await api_post(
|
||||
ironclaw_server,
|
||||
"/api/extensions/install",
|
||||
json={"name": "gmail"},
|
||||
timeout=180,
|
||||
)
|
||||
assert r.status_code == 200, f"Gmail install failed: {r.text}"
|
||||
assert r.json().get("success") is True, f"Install failed: {r.json().get('message', '')}"
|
||||
|
||||
yield
|
||||
|
||||
# Teardown: remove gmail
|
||||
r = await api_post(ironclaw_server, "/api/extensions/gmail/remove", timeout=30)
|
||||
assert r.status_code == 200, f"Gmail removal failed: {r.text}"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
async def auth_url(ironclaw_server, installed_gmail):
|
||||
"""Generate and return an OAuth auth URL.
|
||||
|
||||
Requires Gmail to be installed (depends on installed_gmail fixture).
|
||||
"""
|
||||
r = await api_post(
|
||||
ironclaw_server,
|
||||
"/api/extensions/gmail/setup",
|
||||
json={"secrets": {}},
|
||||
timeout=30,
|
||||
)
|
||||
assert r.status_code == 200
|
||||
data = r.json()
|
||||
assert data.get("success") is True, f"Setup failed: {data.get('message', '')}"
|
||||
|
||||
url = data.get("auth_url")
|
||||
assert url is not None, f"Expected auth_url in response: {data}"
|
||||
assert "accounts.google.com" in url, f"auth_url should point to Google: {url}"
|
||||
|
||||
return url
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
async def oauth_params(auth_url):
|
||||
"""Extract and return OAuth parameters from auth_url.
|
||||
|
||||
Depends on auth_url fixture.
|
||||
"""
|
||||
return await _extract_oauth_params(auth_url)
|
||||
|
||||
|
||||
# ─ OAuth URL parameter validation tests ────────────────────────────────
|
||||
|
||||
async def test_oauth_url_has_client_id_not_clientid(oauth_params, auth_url):
|
||||
"""Verify OAuth URL has 'client_id' (with underscore), NOT 'clientid'.
|
||||
|
||||
Bug #992: Ensure the parameter name is correct across all channels.
|
||||
"""
|
||||
params = oauth_params
|
||||
|
||||
# The bug: "clientid" appears instead of "client_id"
|
||||
# Verify the CORRECT parameter name exists
|
||||
assert "client_id" in params, (
|
||||
f"OAuth URL missing 'client_id' parameter. "
|
||||
f"URL: {auth_url}\nParams: {params}"
|
||||
)
|
||||
assert params["client_id"], "client_id should have a value"
|
||||
|
||||
# Verify the INCORRECT parameter name does NOT exist
|
||||
assert "clientid" not in params, (
|
||||
f"OAuth URL should NOT have 'clientid' (without underscore). "
|
||||
f"Bug #992: URL: {auth_url}\nParams: {params}"
|
||||
)
|
||||
|
||||
|
||||
async def test_oauth_url_has_required_parameters(oauth_params):
|
||||
"""Verify all required OAuth 2.0 parameters are present."""
|
||||
params = oauth_params
|
||||
|
||||
# Required OAuth 2.0 parameters
|
||||
required = ["client_id", "response_type", "redirect_uri", "scope", "state"]
|
||||
for param in required:
|
||||
assert param in params, (
|
||||
f"Missing required OAuth parameter: {param}. "
|
||||
f"Params: {params}"
|
||||
)
|
||||
assert params[param], f"Parameter '{param}' should have a non-empty value"
|
||||
|
||||
# Validate specific values
|
||||
assert params["response_type"] == "code", "Should use authorization_code flow"
|
||||
assert "oauth" in params["redirect_uri"], "Redirect URI should be an OAuth callback"
|
||||
|
||||
|
||||
async def test_oauth_url_has_extra_params(oauth_params):
|
||||
"""Verify extra_params from capabilities.json are included."""
|
||||
params = oauth_params
|
||||
|
||||
# Google-specific extra_params from gmail-tool.capabilities.json
|
||||
assert "access_type" in params, (
|
||||
"Should include 'access_type' from extra_params"
|
||||
)
|
||||
assert params["access_type"] == "offline", (
|
||||
"access_type should be 'offline' for Gmail"
|
||||
)
|
||||
|
||||
assert "prompt" in params, (
|
||||
"Should include 'prompt' from extra_params"
|
||||
)
|
||||
assert params["prompt"] == "consent", (
|
||||
"prompt should be 'consent' for Gmail"
|
||||
)
|
||||
|
||||
|
||||
async def test_oauth_url_is_valid_google_oauth(auth_url):
|
||||
"""Verify the URL is a valid Google OAuth 2.0 authorization URL."""
|
||||
# Verify scheme and host
|
||||
parsed = urlparse(auth_url)
|
||||
assert parsed.scheme == "https", "OAuth URL must use HTTPS"
|
||||
assert "accounts.google.com" in parsed.netloc, "Must be Google's OAuth endpoint"
|
||||
assert parsed.path == "/o/oauth2/v2/auth", "Must use Google OAuth 2.0 endpoint"
|
||||
|
||||
|
||||
async def test_oauth_url_state_is_unique(ironclaw_server, installed_gmail, oauth_params, auth_url):
|
||||
"""Verify CSRF state is present and unique per request."""
|
||||
# Get a new OAuth URL
|
||||
r = await api_post(
|
||||
ironclaw_server,
|
||||
"/api/extensions/gmail/setup",
|
||||
json={"secrets": {}},
|
||||
timeout=30,
|
||||
)
|
||||
assert r.status_code == 200
|
||||
new_auth_url = r.json().get("auth_url")
|
||||
assert new_auth_url is not None
|
||||
|
||||
# Extract state from both URLs
|
||||
original_params = oauth_params
|
||||
new_params = await _extract_oauth_params(new_auth_url)
|
||||
|
||||
original_state = original_params.get("state")
|
||||
new_state = new_params.get("state")
|
||||
|
||||
assert original_state is not None, "Should have state parameter"
|
||||
assert new_state is not None, "New request should have state parameter"
|
||||
assert original_state != new_state, (
|
||||
"CSRF state should be unique per request (for security)"
|
||||
)
|
||||
|
||||
|
||||
async def test_oauth_url_escaping(auth_url):
|
||||
"""Verify URL query parameters are properly escaped."""
|
||||
# Verify special characters in values are URL-encoded
|
||||
# For example, scopes contain spaces which should be %20
|
||||
assert "%20" in auth_url or "+" in auth_url or "%2B" in auth_url or " " not in auth_url, (
|
||||
"OAuth URL should properly encode special characters in parameters"
|
||||
)
|
||||
|
||||
|
||||
# ─ Telegram-specific tests (when Telegram channel is available) ──────────
|
||||
|
||||
class TestOAuthURLViaTelegram:
|
||||
"""Test OAuth URL generation specifically via Telegram channel.
|
||||
|
||||
These tests would verify that the same OAuth URL works correctly when
|
||||
transmitted through the Telegram WASM channel (as opposed to web gateway).
|
||||
|
||||
Currently marked as xfail pending Telegram channel setup in E2E tests.
|
||||
"""
|
||||
|
||||
@pytest.mark.skip(reason="Telegram channel E2E setup not yet implemented")
|
||||
async def test_telegram_oauth_url_has_correct_parameters(self):
|
||||
"""Verify OAuth URL sent via Telegram has correct parameter names."""
|
||||
# This test would:
|
||||
# 1. Send a message via Telegram that triggers OAuth
|
||||
# 2. Capture the status update sent to Telegram
|
||||
# 3. Extract the auth_url from the message
|
||||
# 4. Verify it has "client_id" not "clientid"
|
||||
pass
|
||||
|
||||
@pytest.mark.skip(reason="Telegram channel E2E setup not yet implemented")
|
||||
async def test_telegram_oauth_url_can_be_regenerated(self):
|
||||
"""Verify OAuth URL can be regenerated when requested via Telegram."""
|
||||
# This test would verify that the bug #992 symptom
|
||||
# "URL cannot be regenerated when asked" is fixed.
|
||||
# If the URL is cached incorrectly, regeneration would fail.
|
||||
pass
|
||||
Reference in New Issue
Block a user