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>
282 lines
9.4 KiB
Markdown
282 lines
9.4 KiB
Markdown
# 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.
|