feat: add config-owned direct skill submit path
Add fixed direct-submit skill loading from configured staged skills and validate directSubmitSkill early so malformed configs fail before routing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
木炎
@@ -0,0 +1,281 @@
|
||||
# Config-Owned Direct Skill Contract Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Validate the `directSubmitSkill` control surface early and prevent malformed direct-skill configs from entering the submit routing path, without changing the current happy-path direct execution behavior.
|
||||
|
||||
**Architecture:** Keep the existing direct-submit runtime and submit-task seam intact for valid configs. Move `directSubmitSkill` format validation into the normal `SgClawSettings` load path so malformed config fails before routing begins, while leaving valid-but-unresolvable `skill.tool` targets as direct runtime errors in the current direct path.
|
||||
|
||||
**Tech Stack:** Rust 2021, `serde` config parsing, current `BrowserMessage::SubmitTask` path, current direct skill runtime, Rust integration tests.
|
||||
|
||||
---
|
||||
|
||||
## Execution Context
|
||||
|
||||
- Follow @superpowers:test-driven-development for the Rust code changes in this plan.
|
||||
- Follow @superpowers:verification-before-completion before claiming any task is done.
|
||||
- Do **not** create a git worktree unless the user explicitly asks. This project prefers staying in the current checkout.
|
||||
- Keep scope tight: this plan does **not** add per-skill dispatch metadata, docs changes, intent classification, or LLM routing changes.
|
||||
|
||||
## File Map
|
||||
|
||||
### Existing files to modify
|
||||
|
||||
- Modify: `src/config/settings.rs`
|
||||
- validate `directSubmitSkill` during config normalization
|
||||
- keep the stored field as `Option<String>` so the current direct runtime API stays stable
|
||||
- Modify: `tests/compat_config_test.rs`
|
||||
- add a failing config-load regression for malformed `directSubmitSkill`
|
||||
- Modify: `tests/agent_runtime_test.rs`
|
||||
- add a failing submit-path regression proving malformed config is rejected before direct routing begins
|
||||
|
||||
### Existing files to read but not broaden
|
||||
|
||||
- Reuse without redesign: `src/agent/mod.rs`
|
||||
- Reuse without redesign: `src/compat/direct_skill_runtime.rs`
|
||||
- Reuse without redesign: `docs/superpowers/specs/2026-04-09-config-owned-direct-skill-dispatch-design.md`
|
||||
|
||||
### No new files expected
|
||||
|
||||
This slice should fit in the existing config and tests surfaces only.
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Validate `directSubmitSkill` Before Submit Routing
|
||||
|
||||
**Files:**
|
||||
- Modify: `tests/compat_config_test.rs`
|
||||
- Modify: `tests/agent_runtime_test.rs`
|
||||
- Modify: `src/config/settings.rs`
|
||||
- Read only: `src/agent/mod.rs`
|
||||
- Read only: `src/compat/direct_skill_runtime.rs`
|
||||
|
||||
- [ ] **Step 1: Write the failing config test for malformed `directSubmitSkill`**
|
||||
|
||||
Add this focused test to `tests/compat_config_test.rs`:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn sgclaw_settings_reject_invalid_direct_submit_skill_format() {
|
||||
let root = std::env::temp_dir().join(format!(
|
||||
"sgclaw-invalid-direct-submit-skill-{}",
|
||||
Uuid::new_v4()
|
||||
));
|
||||
fs::create_dir_all(&root).unwrap();
|
||||
let config_path = root.join("sgclaw_config.json");
|
||||
|
||||
fs::write(
|
||||
&config_path,
|
||||
r#"{
|
||||
"providers": [],
|
||||
"skillsDir": "skill_lib",
|
||||
"directSubmitSkill": "fault-details-report"
|
||||
}"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let err = SgClawSettings::load(Some(config_path.as_path()))
|
||||
.expect_err("expected invalid directSubmitSkill format");
|
||||
let message = err.to_string();
|
||||
|
||||
assert!(message.contains("directSubmitSkill"));
|
||||
assert!(message.contains("skill.tool"));
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the focused config test and verify it fails**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test compat_config_test sgclaw_settings_reject_invalid_direct_submit_skill_format -- --nocapture
|
||||
```
|
||||
|
||||
Expected: FAIL because the current config loader accepts the malformed string instead of rejecting it early.
|
||||
|
||||
- [ ] **Step 3: Write the failing agent regression for malformed config**
|
||||
|
||||
Add this focused test to `tests/agent_runtime_test.rs`:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn submit_task_rejects_invalid_direct_submit_skill_config_before_routing() {
|
||||
std::env::remove_var("DEEPSEEK_API_KEY");
|
||||
std::env::remove_var("DEEPSEEK_BASE_URL");
|
||||
std::env::remove_var("DEEPSEEK_MODEL");
|
||||
|
||||
let skill_root = build_direct_runtime_skill_root();
|
||||
let workspace_root = std::env::temp_dir().join(format!(
|
||||
"sgclaw-invalid-direct-submit-workspace-{}",
|
||||
Uuid::new_v4()
|
||||
));
|
||||
fs::create_dir_all(&workspace_root).unwrap();
|
||||
let config_path = workspace_root.join("sgclaw_config.json");
|
||||
fs::write(
|
||||
&config_path,
|
||||
serde_json::json!({
|
||||
"providers": [],
|
||||
"skillsDir": skill_root,
|
||||
"directSubmitSkill": "fault-details-report"
|
||||
})
|
||||
.to_string(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let runtime_context = AgentRuntimeContext::new(Some(config_path), workspace_root);
|
||||
let transport = Arc::new(MockTransport::new(vec![]));
|
||||
let browser_tool = BrowserPipeTool::new(
|
||||
transport.clone(),
|
||||
direct_runtime_test_policy(),
|
||||
vec![1, 2, 3, 4, 5, 6, 7, 8],
|
||||
)
|
||||
.with_response_timeout(Duration::from_secs(1));
|
||||
|
||||
handle_browser_message_with_context(
|
||||
transport.as_ref(),
|
||||
&browser_tool,
|
||||
&runtime_context,
|
||||
submit_fault_details_message(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let sent = transport.sent_messages();
|
||||
assert!(matches!(
|
||||
sent.last(),
|
||||
Some(AgentMessage::TaskComplete { success, summary })
|
||||
if !success && summary.contains("skill.tool")
|
||||
));
|
||||
assert!(direct_submit_mode_logs(&sent).is_empty());
|
||||
assert!(!sent.iter().any(|message| matches!(message, AgentMessage::Command { .. })));
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run the focused agent test and verify it fails**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test agent_runtime_test submit_task_rejects_invalid_direct_submit_skill_config_before_routing -- --nocapture
|
||||
```
|
||||
|
||||
Expected: FAIL because the malformed config currently loads, enters the direct-submit branch, and emits `direct_skill_primary` before failing later.
|
||||
|
||||
- [ ] **Step 5: Implement the minimal config validation**
|
||||
|
||||
In `src/config/settings.rs`, add a small helper that validates the normalized `directSubmitSkill` string during `SgClawSettings::new(...)`.
|
||||
|
||||
Recommended implementation shape:
|
||||
|
||||
```rust
|
||||
fn normalize_direct_submit_skill(raw: Option<String>) -> Result<Option<String>, ConfigError> {
|
||||
let value = normalize_optional_value(raw);
|
||||
let Some(value) = value.as_deref() else {
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
let Some((skill_name, tool_name)) = value.split_once('.') else {
|
||||
return Err(ConfigError::InvalidValue(
|
||||
"directSubmitSkill",
|
||||
format!("must use skill.tool format, got {value}"),
|
||||
));
|
||||
};
|
||||
|
||||
if skill_name.trim().is_empty() || tool_name.trim().is_empty() {
|
||||
return Err(ConfigError::InvalidValue(
|
||||
"directSubmitSkill",
|
||||
format!("must use skill.tool format, got {value}"),
|
||||
));
|
||||
}
|
||||
|
||||
Ok(Some(value.to_string()))
|
||||
}
|
||||
```
|
||||
|
||||
Then use it here:
|
||||
|
||||
```rust
|
||||
let direct_submit_skill = normalize_direct_submit_skill(direct_submit_skill)?;
|
||||
```
|
||||
|
||||
Rules:
|
||||
- do not change the public field type from `Option<String>`
|
||||
- do not move parsing responsibility into `src/agent/mod.rs`
|
||||
- do not redesign `src/compat/direct_skill_runtime.rs`
|
||||
- keep valid-but-unresolvable `skill.tool` targets as runtime errors in the direct path
|
||||
|
||||
- [ ] **Step 6: Re-run the two focused tests and verify they pass**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test compat_config_test sgclaw_settings_reject_invalid_direct_submit_skill_format -- --nocapture
|
||||
cargo test --test agent_runtime_test submit_task_rejects_invalid_direct_submit_skill_config_before_routing -- --nocapture
|
||||
```
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 7: Re-run the broader regression suites**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test compat_config_test -- --nocapture
|
||||
cargo test --test agent_runtime_test -- --nocapture
|
||||
cargo test --test browser_script_skill_tool_test -- --nocapture
|
||||
cargo build --bin sgclaw
|
||||
```
|
||||
|
||||
Expected: PASS, including:
|
||||
- the direct-submit happy path
|
||||
- the existing no-LLM fallback behavior when `directSubmitSkill` is absent
|
||||
- unchanged browser-script helper semantics
|
||||
- clean binary build
|
||||
|
||||
---
|
||||
|
||||
## Verification Checklist
|
||||
|
||||
### Config validation
|
||||
|
||||
```bash
|
||||
cargo test --test compat_config_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected: malformed `directSubmitSkill` is rejected early, while the existing direct-only config shape still loads.
|
||||
|
||||
### Submit-path behavior
|
||||
|
||||
```bash
|
||||
cargo test --test agent_runtime_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected:
|
||||
- malformed `directSubmitSkill` never reaches direct routing
|
||||
- valid configured direct skill still succeeds without LLM config
|
||||
- no direct skill configured still returns the existing no-LLM message
|
||||
|
||||
### Browser-script helper safety
|
||||
|
||||
```bash
|
||||
cargo test --test browser_script_skill_tool_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected: current browser-script execution semantics remain unchanged.
|
||||
|
||||
### Build
|
||||
|
||||
```bash
|
||||
cargo build --bin sgclaw
|
||||
```
|
||||
|
||||
Expected: the main binary compiles cleanly.
|
||||
|
||||
---
|
||||
|
||||
## Notes For The Engineer
|
||||
|
||||
- The paired spec is `docs/superpowers/specs/2026-04-09-config-owned-direct-skill-dispatch-design.md`.
|
||||
- Do **not** add sgClaw-specific dispatch metadata under `D:/data/ideaSpace/rust/sgClaw/claw/claw/skills/skill_staging` in this slice.
|
||||
- Do **not** turn this into a per-skill registry task yet. This plan only hardens the current config-owned bootstrap contract.
|
||||
- Keep the current direct target example as `fault-details-report.collect_fault_details`; avoid hard-coding that name into new generic APIs.
|
||||
- If you discover a need for broader policy routing (`direct_browser` / `llm_agent` by skill), stop and write a new spec/plan instead of expanding this one.
|
||||
@@ -0,0 +1,520 @@
|
||||
# Direct Skill Invocation Without LLM Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Let the current pipe submit-task flow accept natural-language input but directly invoke one fixed staged browser skill without calling any model, while reserving a clean switch back to LLM-based routing later.
|
||||
|
||||
**Architecture:** Keep the existing `BrowserMessage::SubmitTask` entrypoint and add one narrow pre-routing seam before the current compat/LLM chain. When a new config field points to a fixed direct-submit skill, sgClaw loads that skill package from the configured external skills root, finds the target `browser_script` tool, executes it through the existing browser-script wrapper, and returns the result directly. When the field is absent, the current behavior stays unchanged. This preserves a future path where each skill can later declare `direct_browser` or `llm_agent` dispatch without rewriting the submit pipeline again.
|
||||
|
||||
**Tech Stack:** Rust 2021, existing `BrowserPipeTool`, current submit-task agent entrypoint, current browser-script skill executor, current sgClaw JSON config loader, `zeroclaw` skill manifest loader.
|
||||
|
||||
---
|
||||
|
||||
## Recommended First Skill
|
||||
|
||||
Use `fault-details-report.collect_fault_details` from:
|
||||
- `D:/data/ideaSpace/rust/sgClaw/claw/claw/skills/skill_staging/scenes/fault-details-report/scene.json`
|
||||
- `D:/data/ideaSpace/rust/sgClaw/claw/claw/skills/skill_staging/skills/fault-details-report/SKILL.toml`
|
||||
- `D:/data/ideaSpace/rust/sgClaw/claw/claw/skills/skill_staging/skills/fault-details-report/scripts/collect_fault_details.js`
|
||||
|
||||
Why this one first:
|
||||
- it is clearly a report/export skill
|
||||
- it exposes exactly one browser-script tool: `collect_fault_details`
|
||||
- it has the smallest contract surface (`period` only)
|
||||
- its current JS is deterministic and simple, so the first slice can focus on plumbing instead of browser scraping complexity
|
||||
|
||||
## Scope Guardrails
|
||||
|
||||
- Do **not** redesign the existing submit-task protocol.
|
||||
- Do **not** remove or rewrite the current LLM/compat path; leave it as the fallback/default path.
|
||||
- Do **not** introduce generic NL intent routing in this slice; this is one fixed direct skill only.
|
||||
- Do **not** modify `third_party/zeroclaw` skill manifest schema in phase 1.
|
||||
- Do **not** add Excel export wiring in the first slice unless a test explicitly requires it.
|
||||
- Do **not** invent a new browser-script execution model; reuse the existing wrapper semantics.
|
||||
|
||||
---
|
||||
|
||||
## File Map
|
||||
|
||||
### Existing files to modify
|
||||
|
||||
- Modify: `src/config/settings.rs`
|
||||
- add a minimal config field for one direct-submit skill name
|
||||
- Modify: `src/agent/mod.rs`
|
||||
- add a narrow pre-routing branch before the current compat/LLM path
|
||||
- Modify: `src/compat/browser_script_skill_tool.rs`
|
||||
- expose the smallest reusable helper for direct browser-script execution
|
||||
- Modify: `src/compat/mod.rs` or the nearest module export surface
|
||||
- export the new narrow direct-skill runtime module if needed
|
||||
- Modify: `tests/compat_config_test.rs`
|
||||
- add config coverage for the new direct-submit field
|
||||
- Modify: `tests/browser_script_skill_tool_test.rs`
|
||||
- add coverage for the reusable direct-execution helper
|
||||
- Modify: `tests/agent_runtime_test.rs`
|
||||
- prove submit-task can bypass the model and directly invoke the fixed skill
|
||||
|
||||
### New files to create
|
||||
|
||||
- Create: `src/compat/direct_skill_runtime.rs`
|
||||
- small runtime for loading one configured skill, resolving one configured tool, deriving minimal args, and executing it directly
|
||||
|
||||
### Files to reuse without changing behavior
|
||||
|
||||
- Reuse: `src/compat/runtime.rs`
|
||||
- Reuse: `src/compat/orchestration.rs`
|
||||
- Reuse: `src/compat/config_adapter.rs`
|
||||
- Reuse: `third_party/zeroclaw/src/skills/mod.rs`
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Add A Minimal Direct-Submit Skill Config Field
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/config/settings.rs`
|
||||
- Modify: `tests/compat_config_test.rs`
|
||||
|
||||
- [ ] **Step 1: Write the failing config test for the new field**
|
||||
|
||||
In `tests/compat_config_test.rs`, add a focused config-load test proving the browser config file can declare one fixed direct-submit skill.
|
||||
|
||||
Test shape:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn sgclaw_settings_load_direct_submit_skill_from_browser_config() {
|
||||
let root = std::env::temp_dir().join(format!("sgclaw-direct-skill-{}", uuid::Uuid::new_v4()));
|
||||
std::fs::create_dir_all(&root).unwrap();
|
||||
let config_path = root.join("sgclaw_config.json");
|
||||
|
||||
std::fs::write(
|
||||
&config_path,
|
||||
r#"{
|
||||
"apiKey": "sk-runtime",
|
||||
"baseUrl": "https://api.deepseek.com",
|
||||
"model": "deepseek-chat",
|
||||
"skillsDir": "D:/data/ideaSpace/rust/sgClaw/claw/claw/skills/skill_staging",
|
||||
"directSubmitSkill": "fault-details-report.collect_fault_details"
|
||||
}"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let settings = sgclaw::config::SgClawSettings::load(Some(config_path.as_path()))
|
||||
.unwrap()
|
||||
.expect("expected sgclaw settings from config file");
|
||||
|
||||
assert_eq!(
|
||||
settings.direct_submit_skill.as_deref(),
|
||||
Some("fault-details-report.collect_fault_details")
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the focused config test and verify it fails**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test compat_config_test sgclaw_settings_load_direct_submit_skill_from_browser_config -- --nocapture
|
||||
```
|
||||
|
||||
Expected: FAIL because the config field does not exist yet.
|
||||
|
||||
- [ ] **Step 3: Implement the minimal config field**
|
||||
|
||||
In `src/config/settings.rs`, add:
|
||||
- `direct_submit_skill: Option<String>` to `SgClawSettings`
|
||||
- `direct_submit_skill: Option<String>` to `RawSgClawSettings`
|
||||
- field normalization in `SgClawSettings::new(...)`
|
||||
|
||||
Recommended JSON key shape:
|
||||
|
||||
```rust
|
||||
#[serde(rename = "directSubmitSkill", alias = "direct_submit_skill", default)]
|
||||
direct_submit_skill: Option<String>,
|
||||
```
|
||||
|
||||
Rules:
|
||||
- trim empty values to `None`
|
||||
- keep `DeepSeekSettings` unchanged for this slice unless a compile error proves it must mirror the field
|
||||
- do not alter unrelated config semantics
|
||||
|
||||
- [ ] **Step 4: Re-run the focused config test**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test compat_config_test sgclaw_settings_load_direct_submit_skill_from_browser_config -- --nocapture
|
||||
```
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 5: Re-run the broader config file tests**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test compat_config_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 6: Commit Task 1**
|
||||
|
||||
```bash
|
||||
git add src/config/settings.rs tests/compat_config_test.rs
|
||||
git commit -m "feat: add direct submit skill config"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Extract A Reusable Browser-Script Direct Execution Helper
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/compat/browser_script_skill_tool.rs`
|
||||
- Modify: `tests/browser_script_skill_tool_test.rs`
|
||||
|
||||
- [ ] **Step 1: Write the first failing helper test**
|
||||
|
||||
In `tests/browser_script_skill_tool_test.rs`, add a focused test proving direct code can execute a packaged browser script without constructing a full `Tool` object first.
|
||||
|
||||
Test shape:
|
||||
|
||||
```rust
|
||||
#[tokio::test]
|
||||
async fn execute_browser_script_tool_runs_packaged_script_with_expected_domain() {
|
||||
// build temp skill script
|
||||
// call the helper directly
|
||||
// assert Action::Eval was sent with wrapped args and normalized domain
|
||||
}
|
||||
```
|
||||
|
||||
Required assertions:
|
||||
- the helper reads the packaged JS file
|
||||
- it wraps args with `const args = ...`
|
||||
- it normalizes URL-like `expected_domain`
|
||||
- it returns the serialized payload string on success
|
||||
|
||||
- [ ] **Step 2: Run the helper test and verify it fails**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test browser_script_skill_tool_test execute_browser_script_tool_runs_packaged_script_with_expected_domain -- --nocapture
|
||||
```
|
||||
|
||||
Expected: FAIL because the helper does not exist yet.
|
||||
|
||||
- [ ] **Step 3: Add the second failing helper test for required-domain validation**
|
||||
|
||||
Add a focused failure-path test proving the helper rejects missing or invalid `expected_domain` before any browser command is sent.
|
||||
|
||||
- [ ] **Step 4: Run the validation test and verify it fails**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test browser_script_skill_tool_test execute_browser_script_tool_rejects_missing_expected_domain -- --nocapture
|
||||
```
|
||||
|
||||
Expected: FAIL because the helper does not exist yet.
|
||||
|
||||
- [ ] **Step 5: Implement the minimal reusable helper**
|
||||
|
||||
In `src/compat/browser_script_skill_tool.rs`, extract the smallest reusable function, for example:
|
||||
|
||||
```rust
|
||||
pub async fn execute_browser_script_tool<T: Transport + 'static>(
|
||||
tool: &SkillTool,
|
||||
skill_root: &Path,
|
||||
browser_tool: BrowserPipeTool<T>,
|
||||
args: Value,
|
||||
) -> anyhow::Result<ToolResult>
|
||||
```
|
||||
|
||||
Rules:
|
||||
- reuse the current path validation, script loading, wrapping, `Action::Eval`, and payload formatting logic already used by `BrowserScriptSkillTool::execute`
|
||||
- do not change outward behavior of `BrowserScriptSkillTool`
|
||||
- keep the helper narrow and browser-script-only
|
||||
|
||||
- [ ] **Step 6: Refactor `BrowserScriptSkillTool::execute` to call the helper**
|
||||
|
||||
Keep existing behavior and tests green while removing duplicate execution logic.
|
||||
|
||||
- [ ] **Step 7: Re-run the browser-script tests**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test browser_script_skill_tool_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 8: Commit Task 2**
|
||||
|
||||
```bash
|
||||
git add src/compat/browser_script_skill_tool.rs tests/browser_script_skill_tool_test.rs
|
||||
git commit -m "refactor: extract direct browser script execution helper"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Add A Narrow Direct Skill Runtime For One Fixed Skill
|
||||
|
||||
**Files:**
|
||||
- Create: `src/compat/direct_skill_runtime.rs`
|
||||
- Modify: `src/compat/mod.rs` or nearest module export point
|
||||
- Reuse: `src/compat/config_adapter.rs`
|
||||
- Reuse: `third_party/zeroclaw/src/skills/mod.rs`
|
||||
|
||||
- [ ] **Step 1: Write the first failing direct-runtime test**
|
||||
|
||||
Add a focused test in `tests/agent_runtime_test.rs` or a new narrow compat test proving code can resolve the configured external skills root, load `fault-details-report`, find `collect_fault_details`, and execute it directly.
|
||||
|
||||
Recommended shape:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn direct_skill_runtime_executes_fault_details_report_without_provider() {
|
||||
// config points at skill_staging root
|
||||
// direct_submit_skill points at fault-details-report.collect_fault_details
|
||||
// browser response returns report-artifact payload
|
||||
// assert no provider/http path is touched
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the focused direct-runtime test and verify it fails**
|
||||
|
||||
Run the narrowest test command for the new test.
|
||||
|
||||
Expected: FAIL because the direct runtime does not exist yet.
|
||||
|
||||
- [ ] **Step 3: Implement `src/compat/direct_skill_runtime.rs`**
|
||||
|
||||
Add a narrow runtime with responsibilities only to:
|
||||
- resolve the configured skills dir with `resolve_skills_dir_from_sgclaw_settings(...)`
|
||||
- load skills from that directory with `load_skills_from_directory(...)`
|
||||
- parse the configured tool name into `skill_name` + `tool_name`
|
||||
- find the matching skill and matching tool
|
||||
- verify `tool.kind == "browser_script"`
|
||||
- derive the minimal argument object
|
||||
- call the new browser-script helper
|
||||
- return the output string or a clear `PipeError`
|
||||
|
||||
Do **not** add generic routing, scenes, or model fallback here.
|
||||
|
||||
- [ ] **Step 4: Keep argument derivation intentionally minimal**
|
||||
|
||||
For the first slice, derive only:
|
||||
- `expected_domain` from `page_url` when present, otherwise fail with a clear message
|
||||
- `period` from the instruction using a narrow deterministic pattern such as `YYYY-MM`
|
||||
|
||||
If the period cannot be derived, return a concise error telling the user to provide it explicitly. Do not guess.
|
||||
|
||||
- [ ] **Step 5: Re-run the focused direct-runtime test**
|
||||
|
||||
Run the same test command again.
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 6: Commit Task 3**
|
||||
|
||||
```bash
|
||||
git add src/compat/direct_skill_runtime.rs src/compat/mod.rs tests/agent_runtime_test.rs
|
||||
git commit -m "feat: add fixed direct skill runtime"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Insert The Pre-Routing Seam In Submit-Task Entry
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/agent/mod.rs`
|
||||
- Modify: `tests/agent_runtime_test.rs`
|
||||
|
||||
- [ ] **Step 1: Write the first failing submit-path bypass test**
|
||||
|
||||
In `tests/agent_runtime_test.rs`, add a focused regression proving that when `directSubmitSkill` is configured, `BrowserMessage::SubmitTask` can succeed without any model/provider being configured.
|
||||
|
||||
Test shape:
|
||||
|
||||
```rust
|
||||
#[test]
|
||||
fn submit_task_uses_direct_skill_mode_without_llm_configuration() {
|
||||
// config contains skillsDir + directSubmitSkill, but no reachable provider
|
||||
// natural-language instruction includes period and page_url
|
||||
// expect TaskComplete success from direct skill result
|
||||
}
|
||||
```
|
||||
|
||||
Required assertions:
|
||||
- task succeeds even if provider would be unavailable
|
||||
- output contains the report artifact payload
|
||||
- no summary like `未配置大语言模型`
|
||||
|
||||
- [ ] **Step 2: Run the bypass test and verify it fails**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test agent_runtime_test submit_task_uses_direct_skill_mode_without_llm_configuration -- --nocapture
|
||||
```
|
||||
|
||||
Expected: FAIL because submit-task still goes into the current LLM-oriented path.
|
||||
|
||||
- [ ] **Step 3: Add the second failing priority test**
|
||||
|
||||
Add one focused test proving the direct-submit branch runs before the existing compat/LLM branch.
|
||||
|
||||
The easiest assertion is that the mode log becomes something new like:
|
||||
- `direct_skill_primary`
|
||||
|
||||
and the normal mode logs do not appear for that turn.
|
||||
|
||||
- [ ] **Step 4: Run the priority test and verify it fails**
|
||||
|
||||
Run the narrow test command for the new test.
|
||||
|
||||
Expected: FAIL because the mode does not exist yet.
|
||||
|
||||
- [ ] **Step 5: Add the narrow pre-routing branch in `src/agent/mod.rs`**
|
||||
|
||||
In `handle_browser_message_with_context(...)`, after config load/logging and before the existing `should_use_primary_orchestration(...)` / `compat::runtime` path:
|
||||
- check `settings.direct_submit_skill`
|
||||
- if present, emit mode log `direct_skill_primary`
|
||||
- call the new direct runtime
|
||||
- send `TaskComplete` and return immediately
|
||||
|
||||
Rules:
|
||||
- if `direct_submit_skill` is absent, keep existing behavior byte-for-byte where possible
|
||||
- do not modify `compat::runtime.rs` or `compat::orchestration.rs` for this slice
|
||||
- do not silently fall through to LLM when direct execution fails; return the direct error clearly so the first slice is debuggable
|
||||
|
||||
- [ ] **Step 6: Re-run the focused submit-path tests**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test agent_runtime_test submit_task_uses_direct_skill_mode_without_llm_configuration -- --nocapture
|
||||
cargo test --test agent_runtime_test direct_skill_mode_logs_direct_skill_primary -- --nocapture
|
||||
```
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 7: Re-run existing no-LLM submit regression coverage**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test agent_runtime_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected: PASS, including existing cases where no direct skill is configured and the old no-LLM failure still applies.
|
||||
|
||||
- [ ] **Step 8: Commit Task 4**
|
||||
|
||||
```bash
|
||||
git add src/agent/mod.rs tests/agent_runtime_test.rs
|
||||
git commit -m "feat: route submit tasks through fixed direct skill mode"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 5: Lock The Future Migration Seam Without Implementing LLM Dispatch Yet
|
||||
|
||||
**Files:**
|
||||
- Modify only if needed: `src/config/settings.rs`
|
||||
- Modify only if needed: `src/compat/direct_skill_runtime.rs`
|
||||
- Reuse: docs/plan only unless code needs one tiny naming fix
|
||||
|
||||
- [ ] **Step 1: Keep the config naming compatible with future per-skill dispatch**
|
||||
|
||||
Document and preserve this future meaning in code naming:
|
||||
- current field: one fixed direct skill for submit-task bootstrap
|
||||
- future model: each skill can declare dispatch mode such as `direct_browser` or `llm_agent`
|
||||
|
||||
Prefer neutral names in helper code like:
|
||||
- `direct skill mode`
|
||||
- `direct submit skill`
|
||||
|
||||
Avoid hard-coding `fault_details` into generic APIs.
|
||||
|
||||
- [ ] **Step 2: Add one small negative test for fallback behavior**
|
||||
|
||||
Add a focused test proving that when `directSubmitSkill` is not configured, submit-task still behaves exactly as before and can still return the existing no-LLM message.
|
||||
|
||||
If an existing test already proves this, keep it and do not add another.
|
||||
|
||||
- [ ] **Step 3: Re-run the focused end-to-end verification set**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo test --test compat_config_test -- --nocapture
|
||||
cargo test --test browser_script_skill_tool_test -- --nocapture
|
||||
cargo test --test agent_runtime_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 4: Build the main binary**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
cargo build --bin sgclaw
|
||||
```
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 5: Commit Task 5**
|
||||
|
||||
```bash
|
||||
git add src/config/settings.rs src/compat/direct_skill_runtime.rs src/compat/browser_script_skill_tool.rs src/agent/mod.rs tests/compat_config_test.rs tests/browser_script_skill_tool_test.rs tests/agent_runtime_test.rs
|
||||
git commit -m "test: verify fixed direct skill submit path"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Verification Checklist
|
||||
|
||||
### Config loading
|
||||
|
||||
```bash
|
||||
cargo test --test compat_config_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected: `directSubmitSkill` loads correctly and existing config behavior remains intact.
|
||||
|
||||
### Browser-script helper
|
||||
|
||||
```bash
|
||||
cargo test --test browser_script_skill_tool_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected: direct helper preserves the existing browser-script execution semantics.
|
||||
|
||||
### Submit-path bypass
|
||||
|
||||
```bash
|
||||
cargo test --test agent_runtime_test -- --nocapture
|
||||
```
|
||||
|
||||
Expected: configured direct skill bypasses the model path, while unconfigured submit-task behavior stays unchanged.
|
||||
|
||||
### Build
|
||||
|
||||
```bash
|
||||
cargo build --bin sgclaw
|
||||
```
|
||||
|
||||
Expected: the binary compiles cleanly.
|
||||
|
||||
---
|
||||
|
||||
## Notes For The Engineer
|
||||
|
||||
- The key to keeping this slice small is to avoid changing `compat::runtime.rs` and `compat::orchestration.rs`; they remain the future LLM path.
|
||||
- `fault-details-report.collect_fault_details` is only the bootstrap skill. The plumbing must stay generic enough that the configured tool name can later point to another staged browser skill.
|
||||
- Phase 1 should not add per-skill dispatch metadata to the external skill manifests yet. Keep that decision in sgClaw config first; move it into skill metadata only after the direct path is proven useful.
|
||||
- Once the intranet model is ready, the clean next step is to add a dispatch policy layer that chooses between `direct_browser` and `llm_agent` before the current compat path is entered, reusing this same pre-routing seam.
|
||||
Reference in New Issue
Block a user