diff --git a/.opencode/commands/forge.md b/.opencode/commands/forge.md index a786f04..ceedccf 100644 --- a/.opencode/commands/forge.md +++ b/.opencode/commands/forge.md @@ -2,10 +2,20 @@ description: Decompose task into subtasks and coordinate parallel agents --- + # /forge Decompose a task and spawn parallel workers. +## Critical Invariants + +These rules are non-negotiable and MUST NOT be skipped: + +- **Review MUST complete before marking work done** — step 7 (review) MUST finish before step 8 (complete). NEVER skip review. +- **The review gate is mandatory** — `forge_complete` MUST NOT be called until `forge_review` has passed for every worker. NEVER bypass the review gate. +- ALWAYS create a forge, even for small tasks. +- Coordinator orchestrates, workers execute — workers MUST NOT call `forge_complete`. + ## Task $ARGUMENTS @@ -15,53 +25,34 @@ $ARGUMENTS 1. Initialize comms: `comms_init(project_path=".", task_description="Forge: ")` 2. Check prior learnings: `hivemind_find(query="")` 3. Decompose: `forge_decompose(task="", context="")` + - Before decomposing, check historical success rates: `forge_get_strategy_insights(task="")` + - Choose from: `file-based`, `feature-based`, `risk-based`, or `auto` 4. Create epic: `org_create_epic(epic_title="", subtasks=[...])` -5. For each subtask: `forge_spawn_subtask(bead_id, epic_id, subtask_title, files)` +5. For each subtask: `forge_spawn_subtask(bead_id, epic_id, subtask_title, files, task_classification, skill | skill_absent_reason)` + - `task_classification` is required: `deterministic` or `judgment` + - `deterministic` subtasks require a `skill` reference (verified skill name or repo-relative script path) OR a `skill_absent_reason` — never both + - `judgment` subtasks must omit both `skill` and `skill_absent_reason` — supplying either fails input validation + - Unverifiable references fail closed (`SKILL_NOT_FOUND`); missing both fails with `SKILL_REFERENCE_REQUIRED` + - Pass `project_path` when spawning against a repo other than the current directory + - Forward the envelope's `skill_directive` verbatim into the worker prompt (or pass the same `skill` to `forge_subtask_prompt`); never write skill instructions yourself 6. Monitor: check `comms_inbox()` every few minutes -7. Review: `forge_review(task_id, files_touched)` for each completed worker -8. Complete: `forge_complete(bead_id, summary, files_touched)` -9. Store learnings: `hivemind_store(information="...", tags="forge,")` + - `comms_inbox()` — check for messages from workers + - `forge_status(epic_id, project_key)` — check worker progress + - `org_cells(status="in_progress")` — see active cells + - If a worker is blocked: read the message with `comms_read_message(message_id)`, acknowledge with `comms_ack(message_id)`, then either unblock or reassign the subtask +7. Review FIRST (before complete): `forge_review(task_id, files_touched)` for each completed worker — MUST finish before step 8 +8. Complete (after ALL reviews pass): + - `forge_complete(bead_id, summary, files_touched)` — mark epic done + - `forge_record_outcome(bead_id, duration_ms, success)` — record for learning + - `hivemind_store(information="...", tags="forge,")` — store learnings + - `org_sync()` — persist state to git ## Rules -- Always create a forge, even for small tasks -- Coordinator orchestrates, workers execute +- Review every worker's output before marking complete — NEVER skip this step +- ALWAYS create a forge, even for small tasks +- Coordinator orchestrates, workers execute — workers MUST NOT call `forge_complete` - Workers reserve their own files via `comms_reserve` - Check inbox regularly for blocked workers -- Review every worker's output before marking complete - Store learnings after completion - -## Strategy Selection - -Before decomposing, check historical success rates: - -``` -forge_get_strategy_insights(task="") -``` - -Choose from: `file-based`, `feature-based`, `risk-based`, or `auto`. - -## Monitoring - -While workers are active: - -1. `comms_inbox()` — check for messages from workers -2. `forge_status(epic_id, project_key)` — check worker progress -3. `org_cells(status="in_progress")` — see active cells - -## Completion - -After all workers finish: - -1. `forge_complete(bead_id, summary, files_touched)` — mark epic done -2. `forge_record_outcome(bead_id, duration_ms, success)` — record for learning -3. `hivemind_store(information="...", tags="forge,")` — store learnings -4. `org_sync()` — persist state to git - -## Error Recovery - -If a worker is blocked: - -1. Read the worker's message: `comms_read_message(message_id)` -2. Acknowledge: `comms_ack(message_id)` -3. Either unblock or reassign the subtask + diff --git a/.opencode/skills/forge-coordination/SKILL.md b/.opencode/skills/forge-coordination/SKILL.md index e8476ab..89427dd 100644 --- a/.opencode/skills/forge-coordination/SKILL.md +++ b/.opencode/skills/forge-coordination/SKILL.md @@ -21,7 +21,10 @@ Patterns for coordinating parallel agent work. 3. **Select strategy**: `forge_get_strategy_insights(task="")` 4. **Decompose**: `forge_decompose(task="", context="")` 5. **Create epic**: `org_create_epic(epic_title="", subtasks=[...])` -6. **Spawn workers**: `forge_spawn_subtask(bead_id, epic_id, subtask_title, files)` +6. **Spawn workers**: `forge_spawn_subtask(bead_id, epic_id, subtask_title, files, task_classification, skill | skill_absent_reason)` + - `task_classification` is required (`deterministic` | `judgment`) + - `deterministic` subtasks must supply a verified `skill` reference or a `skill_absent_reason` (mutually exclusive); spawn fails closed with `SKILL_REFERENCE_REQUIRED` / `SKILL_NOT_FOUND` + - `judgment` subtasks must omit both `skill` and `skill_absent_reason`; forward the envelope's `skill_directive` verbatim (or pass the same `skill` to `forge_subtask_prompt`) — never write skill instructions yourself 7. **Monitor**: `comms_inbox()` + `forge_status(epic_id, project_key)` 8. **Review**: `forge_review(task_id, files_touched)` for each worker 9. **Complete**: `forge_complete(bead_id, summary, files_touched)` diff --git a/CHANGELOG.md b/CHANGELOG.md index a80ae08..3c312ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,19 @@ this project adheres to [Semantic Versioning](https://semver.org/). (Part of unbound-force/unbound-force#428) - Slash commands migrated to `uf.*` namespace (e.g., `/unleash` is now `/uf.unleash`, `/cobalt-crush` is now `/uf.cobalt-crush`) +- **BREAKING**: `forge_spawn_subtask` now enforces the delegation + contract: `task_classification` is required (`deterministic` or + `judgment`); deterministic-classified subtasks must supply a + `skill` reference (1-256 chars, existence-verified) or a + `skill_absent_reason` (1-512 chars) — never both; judgment-classified + subtasks must omit both. Violations fail closed with + `SKILL_REFERENCE_REQUIRED` or `SKILL_NOT_FOUND`. The success + envelope gains `task_classification`, the resolved skill path, + `skill_absent_reason`, and the tool-rendered `skill_directive`; + `forge_subtask_prompt` gains an optional `skill` parameter that + embeds the same fixed directive. + (Fixes [#143](https://github.com/unbound-force/replicator/issues/143)) + - Spec: `openspec/changes/spawn-subtask-skill-enforcement/specs/spawn-delegation-contract/spec.md` ### Security - Minimum Go toolchain raised to 1.26.9 to resolve reachable standard-library diff --git a/docs/tools.md b/docs/tools.md index daffdff..06a3045 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -1032,7 +1032,7 @@ Analyze task and recommend decomposition strategy. ### `forge_spawn_subtask` -Prepare a subtask for spawning with Task tool. +Prepare a subtask for spawning with Task tool. Delegation contract: task_classification is required (deterministic|judgment); deterministic-classified subtasks must supply a verified skill/script reference (skill) or a recorded skill_absent_reason (mutually exclusive); judgment-classified subtasks must omit both. Violations fail closed with SKILL_REFERENCE_REQUIRED or SKILL_NOT_FOUND. ```json { @@ -1049,21 +1049,43 @@ Prepare a subtask for spawning with Task tool. }, "type": "array" }, + "project_path": { + "default": ".", + "type": "string" + }, "shared_context": { "type": "string" }, + "skill": { + "maxLength": 256, + "minLength": 1, + "type": "string" + }, + "skill_absent_reason": { + "maxLength": 512, + "minLength": 1, + "type": "string" + }, "subtask_description": { "type": "string" }, "subtask_title": { "type": "string" + }, + "task_classification": { + "enum": [ + "deterministic", + "judgment" + ], + "type": "string" } }, "required": [ "bead_id", "epic_id", "subtask_title", - "files" + "files", + "task_classification" ], "type": "object" } @@ -1093,7 +1115,7 @@ Get status of a swarm by epic ID. ### `forge_subtask_prompt` -Generate the prompt for a spawned subtask agent. +Generate the prompt for a spawned subtask agent. When skill is supplied, the fixed skill directive is embedded in the prompt. ```json { @@ -1116,6 +1138,9 @@ Generate the prompt for a spawned subtask agent. "shared_context": { "type": "string" }, + "skill": { + "type": "string" + }, "subtask_description": { "type": "string" }, diff --git a/internal/agentkit/agentkit_test.go b/internal/agentkit/agentkit_test.go index 9eadde9..3dbaa13 100644 --- a/internal/agentkit/agentkit_test.go +++ b/internal/agentkit/agentkit_test.go @@ -836,6 +836,41 @@ func TestForgeMD_StructuralHardening(t *testing.T) { }) } +// TestAgentKit_DelegationContractDemonstrated verifies the embedded +// kit demonstrates the forge_spawn_subtask delegation contract: +// the coordinator agent, the /forge command, and the +// forge-coordination skill all show task_classification, the +// reference-or-reason rule, the judgment-side prohibition, and the +// tool-rendered skill_directive forwarding rule. +func TestAgentKit_DelegationContractDemonstrated(t *testing.T) { + cases := []struct { + path string + }{ + {"content/agents/coordinator.md"}, + {"content/commands/forge.md"}, + {"content/skills/forge-coordination/SKILL.md"}, + } + for _, tc := range cases { + data, err := content.ReadFile(tc.path) + if err != nil { + t.Fatalf("read %s: %v", tc.path, err) + } + text := string(data) + for _, needle := range []string{ + "forge_spawn_subtask", + "task_classification", + "skill_absent_reason", + "SKILL_REFERENCE_REQUIRED", + "omit both", + "skill_directive", + } { + if !strings.Contains(text, needle) { + t.Errorf("%s: delegation contract marker %q not found", tc.path, needle) + } + } + } +} + func TestForgeCoordinationSkill_StructuralHardening(t *testing.T) { data, err := content.ReadFile("content/skills/forge-coordination/SKILL.md") if err != nil { diff --git a/internal/agentkit/content/agents/coordinator.md b/internal/agentkit/content/agents/coordinator.md index 985aa87..e7f4d02 100644 --- a/internal/agentkit/content/agents/coordinator.md +++ b/internal/agentkit/content/agents/coordinator.md @@ -19,7 +19,16 @@ You orchestrate workers but NEVER reserve files or edit code directly. You decom 1. Initialize comms (`comms_init`) 2. Decompose task via `forge_decompose` or `forge_plan_prompt` -3. Spawn workers with `forge_spawn_subtask` +3. Spawn workers with `forge_spawn_subtask` — every call MUST pass + `task_classification` (`deterministic` or `judgment`); a + `deterministic` subtask additionally requires either a verified + `skill` reference (skill name or repo-relative script path) or a + recorded `skill_absent_reason` (mutually exclusive). Spawn fails + closed with `SKILL_REFERENCE_REQUIRED` / `SKILL_NOT_FOUND` + otherwise. A `judgment` subtask MUST omit both `skill` and + `skill_absent_reason`. Forward the envelope's `skill_directive` + verbatim into the worker prompt (or pass the same `skill` to + `forge_subtask_prompt`); never write skill instructions yourself. 4. Check inbox regularly for blocked workers (`comms_inbox`) 5. Use `forge_broadcast` to share context updates with all workers 6. Review every worker completion (`forge_review`) diff --git a/internal/agentkit/content/commands/forge.md b/internal/agentkit/content/commands/forge.md index bfd7b78..ceedccf 100644 --- a/internal/agentkit/content/commands/forge.md +++ b/internal/agentkit/content/commands/forge.md @@ -28,7 +28,13 @@ $ARGUMENTS - Before decomposing, check historical success rates: `forge_get_strategy_insights(task="")` - Choose from: `file-based`, `feature-based`, `risk-based`, or `auto` 4. Create epic: `org_create_epic(epic_title="", subtasks=[...])` -5. For each subtask: `forge_spawn_subtask(bead_id, epic_id, subtask_title, files)` +5. For each subtask: `forge_spawn_subtask(bead_id, epic_id, subtask_title, files, task_classification, skill | skill_absent_reason)` + - `task_classification` is required: `deterministic` or `judgment` + - `deterministic` subtasks require a `skill` reference (verified skill name or repo-relative script path) OR a `skill_absent_reason` — never both + - `judgment` subtasks must omit both `skill` and `skill_absent_reason` — supplying either fails input validation + - Unverifiable references fail closed (`SKILL_NOT_FOUND`); missing both fails with `SKILL_REFERENCE_REQUIRED` + - Pass `project_path` when spawning against a repo other than the current directory + - Forward the envelope's `skill_directive` verbatim into the worker prompt (or pass the same `skill` to `forge_subtask_prompt`); never write skill instructions yourself 6. Monitor: check `comms_inbox()` every few minutes - `comms_inbox()` — check for messages from workers - `forge_status(epic_id, project_key)` — check worker progress diff --git a/internal/agentkit/content/skills/forge-coordination/SKILL.md b/internal/agentkit/content/skills/forge-coordination/SKILL.md index e8476ab..89427dd 100644 --- a/internal/agentkit/content/skills/forge-coordination/SKILL.md +++ b/internal/agentkit/content/skills/forge-coordination/SKILL.md @@ -21,7 +21,10 @@ Patterns for coordinating parallel agent work. 3. **Select strategy**: `forge_get_strategy_insights(task="")` 4. **Decompose**: `forge_decompose(task="", context="")` 5. **Create epic**: `org_create_epic(epic_title="", subtasks=[...])` -6. **Spawn workers**: `forge_spawn_subtask(bead_id, epic_id, subtask_title, files)` +6. **Spawn workers**: `forge_spawn_subtask(bead_id, epic_id, subtask_title, files, task_classification, skill | skill_absent_reason)` + - `task_classification` is required (`deterministic` | `judgment`) + - `deterministic` subtasks must supply a verified `skill` reference or a `skill_absent_reason` (mutually exclusive); spawn fails closed with `SKILL_REFERENCE_REQUIRED` / `SKILL_NOT_FOUND` + - `judgment` subtasks must omit both `skill` and `skill_absent_reason`; forward the envelope's `skill_directive` verbatim (or pass the same `skill` to `forge_subtask_prompt`) — never write skill instructions yourself 7. **Monitor**: `comms_inbox()` + `forge_status(epic_id, project_key)` 8. **Review**: `forge_review(task_id, files_touched)` for each worker 9. **Complete**: `forge_complete(bead_id, summary, files_touched)` diff --git a/internal/forge/spawn.go b/internal/forge/spawn.go index 3e0bdd5..cb3f93b 100644 --- a/internal/forge/spawn.go +++ b/internal/forge/spawn.go @@ -2,16 +2,68 @@ package forge import ( "encoding/json" + "errors" "fmt" + "io/fs" + "os" + "path/filepath" "strings" "time" "github.com/unbound-force/replicator/internal/db" ) +// Sentinel errors for the spawn delegation contract. The codes lead +// the wrapped error messages so failures stay greppable in tool +// output and logs; match with errors.Is. +var ( + // ErrSkillReferenceRequired fails a deterministic-classified + // spawn that supplies neither a skill/script reference nor a + // recorded skill_absent_reason. + ErrSkillReferenceRequired = errors.New("SKILL_REFERENCE_REQUIRED") + + // ErrSkillNotFound fails a spawn whose skill/script reference + // cannot be resolved to an existing file under the project. + ErrSkillNotFound = errors.New("SKILL_NOT_FOUND") +) + +// Task classification values for the spawn delegation contract. +const ( + ClassificationDeterministic = "deterministic" + ClassificationJudgment = "judgment" +) + +// Length bounds for delegation contract fields. +const ( + MaxSkillRefLen = 256 + MaxSkillReasonLen = 512 +) + +// SpawnParams carries the inputs for spawning a subtask, +// including the delegation contract parameters +// (task_classification, skill, skill_absent_reason) and the +// optional ProjectPath used to resolve skill references +// (empty means the working directory). +type SpawnParams struct { + BeadID string `json:"bead_id"` + EpicID string `json:"epic_id"` + Title string `json:"subtask_title"` + Files []string `json:"files"` + Description string `json:"subtask_description"` + SharedContext string `json:"shared_context"` + TaskClassification string `json:"task_classification"` + Skill string `json:"skill"` + SkillAbsentReason string `json:"skill_absent_reason"` + ProjectPath string `json:"project_path"` +} + // SubtaskPrompt generates a prompt for a spawned subtask agent. -// This is a prompt generator -- it does NOT call an LLM. -func SubtaskPrompt(agentName, beadID, epicID, title string, files []string, sharedContext string) string { +// This is a prompt generator -- it does NOT call an LLM. When +// skillRef is non-empty the fixed skill directive is rendered so +// the worker sees the same instructions the spawn envelope +// recorded; otherwise the output is unchanged from the +// no-reference shape. +func SubtaskPrompt(agentName, beadID, epicID, title string, files []string, sharedContext, skillRef string) string { var sb strings.Builder _, _ = fmt.Fprintf(&sb, "# Subtask: %s\n\n", title) @@ -33,6 +85,12 @@ func SubtaskPrompt(agentName, beadID, epicID, title string, files []string, shar sb.WriteString("\n\n") } + if skillRef != "" { + sb.WriteString("## Required Skill\n\n") + sb.WriteString(SkillDirective(skillRef)) + sb.WriteString("\n\n") + } + sb.WriteString("## Instructions\n\n") sb.WriteString("1. Reserve files before editing using `comms_reserve`\n") sb.WriteString("2. Implement the changes described above\n") @@ -45,31 +103,191 @@ func SubtaskPrompt(agentName, beadID, epicID, title string, files []string, shar // SpawnSubtask prepares metadata for spawning a subtask agent. // Returns the metadata needed to launch the agent. -func SpawnSubtask(beadID, epicID, title string, files []string, description, sharedContext string) (map[string]any, error) { - if beadID == "" { +// +// It enforces the spawn delegation contract: task_classification +// is required and enum-checked, skill and skill_absent_reason are +// mutually exclusive and valid only for deterministic +// classification, and a deterministic-classified subtask must +// supply one or the other (ErrSkillReferenceRequired otherwise). +// A non-empty skill is resolved against the project root +// (ProjectPath, defaulting to the working directory): an +// unresolvable reference fails with ErrSkillNotFound, while other +// filesystem errors (for example permission denied) are returned +// unwrapped so operational failures stay distinguishable from +// absence. Empty-string Skill and SkillAbsentReason values are +// treated as absent at this layer; the MCP tool boundary rejects +// explicit empty strings. +func SpawnSubtask(params SpawnParams) (map[string]any, error) { + if params.BeadID == "" { return nil, fmt.Errorf("bead_id is required") } - if epicID == "" { + if params.EpicID == "" { return nil, fmt.Errorf("epic_id is required") } - if title == "" { + if params.Title == "" { return nil, fmt.Errorf("title is required") } + if err := validateSpawnContract(¶ms); err != nil { + return nil, err + } result := map[string]any{ - "status": "ready_to_spawn", - "bead_id": beadID, - "epic_id": epicID, - "title": title, - "files": files, - "description": description, - "shared_context": sharedContext, - "timestamp": time.Now().UTC().Format(time.RFC3339), + "status": "ready_to_spawn", + "bead_id": params.BeadID, + "epic_id": params.EpicID, + "title": params.Title, + "files": params.Files, + "description": params.Description, + "shared_context": params.SharedContext, + "task_classification": params.TaskClassification, + "timestamp": time.Now().UTC().Format(time.RFC3339), + } + + if params.Skill != "" { + resolved, err := ResolveSkillRef(params.ProjectPath, params.Skill) + if err != nil { + return nil, err + } + result["skill"] = resolved + result["skill_directive"] = SkillDirective(params.Skill) + } + if params.SkillAbsentReason != "" { + result["skill_absent_reason"] = params.SkillAbsentReason } return result, nil } +// validateSpawnContract enforces the structural rules of the spawn +// delegation contract: classification is required and enum-checked, +// skill and skill_absent_reason are mutually exclusive, both are +// valid only for deterministic classification, and length bounds +// hold. Existence resolution happens after these checks pass. +func validateSpawnContract(p *SpawnParams) error { + switch p.TaskClassification { + case ClassificationDeterministic, ClassificationJudgment: + default: + if p.TaskClassification == "" { + return fmt.Errorf("task_classification is required (accepted values: deterministic, judgment)") + } + return fmt.Errorf("task_classification must be deterministic or judgment (got %q)", p.TaskClassification) + } + + if p.Skill != "" && p.SkillAbsentReason != "" { + return fmt.Errorf("skill and skill_absent_reason are mutually exclusive; supply one or the other, not both") + } + + if p.TaskClassification == ClassificationJudgment { + if p.Skill != "" { + return fmt.Errorf("skill is valid only for deterministic classification (got %q); omit it for judgment work", p.TaskClassification) + } + if p.SkillAbsentReason != "" { + return fmt.Errorf("skill_absent_reason is valid only for deterministic classification (got %q); omit it for judgment work", p.TaskClassification) + } + } + + if p.Skill != "" && len(p.Skill) > MaxSkillRefLen { + return fmt.Errorf("skill must be at most %d characters (got %d)", MaxSkillRefLen, len(p.Skill)) + } + if p.SkillAbsentReason != "" && len(p.SkillAbsentReason) > MaxSkillReasonLen { + return fmt.Errorf("skill_absent_reason must be at most %d characters (got %d)", MaxSkillReasonLen, len(p.SkillAbsentReason)) + } + + if p.TaskClassification == ClassificationDeterministic && p.Skill == "" && p.SkillAbsentReason == "" { + return fmt.Errorf("%w: deterministic-classified subtask requires a verified skill/script reference or a recorded skill_absent_reason; supply skill or skill_absent_reason", ErrSkillReferenceRequired) + } + + return nil +} + +// skillRoots lists, in order, the repo-relative directories under +// which a bare skill name resolves to a SKILL.md file. The first +// root containing the skill wins. +var skillRoots = []string{ + filepath.Join(".opencode", "skills"), + filepath.Join(".opencode", "skill"), +} + +// ResolveSkillRef resolves a skill/script reference against the +// project root. A value without a path separator resolves as a +// skill name against the project skill directories; a value +// containing a separator resolves as a repo-relative file path +// (absolute paths and traversal outside the root are rejected). +// The returned path is repo-relative. +func ResolveSkillRef(projectPath, ref string) (string, error) { + if projectPath == "" { + projectPath = "." + } + if ref == "." || ref == ".." { + return "", fmt.Errorf("%w: path traversal is not allowed (reference %q must stay inside the project root)", ErrSkillNotFound, ref) + } + if strings.ContainsRune(ref, '/') { + return resolveRepoRelative(projectPath, ref) + } + var locations []string + for _, root := range skillRoots { + rel := filepath.Join(root, ref, "SKILL.md") + exists, err := fileExists(filepath.Join(projectPath, rel)) + if err != nil { + return "", fmt.Errorf("resolving skill %q under %s: %w", ref, projectPath, err) + } + if exists { + return rel, nil + } + locations = append(locations, rel) + } + return "", fmt.Errorf("%w: skill %q not found (searched: %s under %s)", ErrSkillNotFound, ref, strings.Join(locations, ", "), projectPath) +} + +// resolveRepoRelative resolves a slash-containing reference as a +// repo-relative path, rejecting absolute paths and any path that +// escapes the project root. +func resolveRepoRelative(projectPath, ref string) (string, error) { + if filepath.IsAbs(ref) { + return "", fmt.Errorf("%w: absolute paths are not allowed (reference %q must be repo-relative)", ErrSkillNotFound, ref) + } + cleaned := filepath.Clean(ref) + if cleaned == ".." || strings.HasPrefix(cleaned, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("%w: path traversal is not allowed (reference %q must stay inside the project root)", ErrSkillNotFound, ref) + } + exists, err := fileExists(filepath.Join(projectPath, cleaned)) + if err != nil { + return "", fmt.Errorf("resolving %q under %s: %w", cleaned, projectPath, err) + } + if !exists { + return "", fmt.Errorf("%w: %s does not exist under %s", ErrSkillNotFound, cleaned, projectPath) + } + return cleaned, nil +} + +// fileExists reports whether path exists and is not a directory. +// A stat error other than not-exist is returned so callers can +// distinguish operational failures (for example permission +// denied) from absence. +func fileExists(path string) (bool, error) { + info, err := os.Stat(path) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return false, nil + } + return false, err + } + return !info.IsDir(), nil +} + +// SkillDirective renders the fixed worker directive for a skill +// or script reference. The wording is a design constant (mirrors +// the parent delegation contract in unbound-force) and is pinned +// by golden-string tests; do not vary it per call site. +func SkillDirective(ref string) string { + return fmt.Sprintf( + "Required skill: %s\n\n"+ + "This task is classified as deterministic work. Load the skill (invoke the `skill` tool with name %q) "+ + "or run the script at %s, and execute its full procedure. Do not substitute an improvised procedure.", + ref, ref, ref, + ) +} + // CompleteSubtask records the completion of a subtask and updates the cell status. func CompleteSubtask(store *db.Store, beadID, taskResult string, filesTouched []string) (map[string]any, error) { if beadID == "" { diff --git a/internal/forge/spawn_test.go b/internal/forge/spawn_test.go index 40d6a88..e19ed4e 100644 --- a/internal/forge/spawn_test.go +++ b/internal/forge/spawn_test.go @@ -1,16 +1,365 @@ package forge import ( + "errors" + "os" + "path/filepath" "strings" "testing" ) +// writeFile creates parent directories and writes a file under dir. +func writeFile(t *testing.T, dir, rel, content string) { + t.Helper() + path := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", path, err) + } + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + +func TestResolveSkillRef_PrimaryRoot(t *testing.T) { + root := t.TempDir() + writeFile(t, root, ".opencode/skills/deploy/SKILL.md", "# deploy") + + got, err := ResolveSkillRef(root, "deploy") + if err != nil { + t.Fatalf("ResolveSkillRef: %v", err) + } + want := filepath.Join(".opencode", "skills", "deploy", "SKILL.md") + if got != want { + t.Errorf("resolved = %q, want %q", got, want) + } +} + +func TestResolveSkillRef_FallbackRoot(t *testing.T) { + root := t.TempDir() + writeFile(t, root, ".opencode/skill/deploy/SKILL.md", "# deploy") + + got, err := ResolveSkillRef(root, "deploy") + if err != nil { + t.Fatalf("ResolveSkillRef: %v", err) + } + want := filepath.Join(".opencode", "skill", "deploy", "SKILL.md") + if got != want { + t.Errorf("resolved = %q, want %q", got, want) + } +} + +func TestResolveSkillRef_FirstRootWins(t *testing.T) { + root := t.TempDir() + writeFile(t, root, ".opencode/skills/deploy/SKILL.md", "# primary") + writeFile(t, root, ".opencode/skill/deploy/SKILL.md", "# fallback") + + got, err := ResolveSkillRef(root, "deploy") + if err != nil { + t.Fatalf("ResolveSkillRef: %v", err) + } + want := filepath.Join(".opencode", "skills", "deploy", "SKILL.md") + if got != want { + t.Errorf("resolved = %q, want %q (primary root must win)", got, want) + } +} + +func TestResolveSkillRef_RepoRelativePath(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "scripts/migrate.sh", "#!/bin/sh") + + got, err := ResolveSkillRef(root, "scripts/migrate.sh") + if err != nil { + t.Fatalf("ResolveSkillRef: %v", err) + } + if got != filepath.Join("scripts", "migrate.sh") { + t.Errorf("resolved = %q, want scripts/migrate.sh", got) + } +} + +func TestResolveSkillRef_AbsolutePathRejected(t *testing.T) { + abs := filepath.Join(t.TempDir(), "scripts", "migrate.sh") + // The absolute target exists: rejection must come from the + // absolute-path guard, not accidental non-existence. + writeFile(t, filepath.Dir(filepath.Dir(abs)), "scripts/migrate.sh", "#!/bin/sh") + + _, err := ResolveSkillRef(t.TempDir(), abs) + if !errors.Is(err, ErrSkillNotFound) { + t.Fatalf("absolute path should fail with ErrSkillNotFound, got: %v", err) + } + if msg := err.Error(); !strings.Contains(msg, "absolute") { + t.Errorf("error should state absolute paths are rejected, got: %s", msg) + } +} + +func TestResolveSkillRef_TraversalRejected(t *testing.T) { + parent := t.TempDir() + root := filepath.Join(parent, "project") + if err := os.MkdirAll(root, 0o755); err != nil { + t.Fatalf("mkdir project root: %v", err) + } + // The traversal target exists outside the root: rejection must + // come from the traversal guard, not accidental non-existence. + writeFile(t, parent, "secret.sh", "#!/bin/sh") + + for _, ref := range []string{"../secret.sh", "a/../../secret.sh", ".."} { + _, err := ResolveSkillRef(root, ref) + if !errors.Is(err, ErrSkillNotFound) { + t.Errorf("traversal ref %q should fail with ErrSkillNotFound, got: %v", ref, err) + } + if msg := err.Error(); !strings.Contains(msg, "traversal") { + t.Errorf("error for %q should mention traversal, got: %s", ref, msg) + } + } +} + +func TestResolveSkillRef_NormalizedPathResolves(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "scripts/migrate.sh", "#!/bin/sh") + + got, err := ResolveSkillRef(root, "scripts/../scripts/migrate.sh") + if err != nil { + t.Fatalf("ResolveSkillRef: %v", err) + } + if got != filepath.Join("scripts", "migrate.sh") { + t.Errorf("resolved = %q, want cleaned scripts/migrate.sh", got) + } +} + +func TestResolveSkillRef_UnreadablePathFailsClosed(t *testing.T) { + root := t.TempDir() + writeFile(t, root, ".opencode/skills/deploy/SKILL.md", "# deploy") + + skillsDir := filepath.Join(root, ".opencode", "skills") + if err := os.Chmod(skillsDir, 0o000); err != nil { + t.Fatalf("chmod skills dir: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(skillsDir, 0o755) }) + + _, err := ResolveSkillRef(root, "deploy") + if err == nil { + t.Fatal("unreadable skill directory should fail closed") + } + if errors.Is(err, ErrSkillNotFound) { + t.Fatalf("permission failure must not be reported as ErrSkillNotFound, got: %v", err) + } +} + +func TestResolveSkillRef_UnknownSkillFailsClosed(t *testing.T) { + root := t.TempDir() + + _, err := ResolveSkillRef(root, "deploy") + if !errors.Is(err, ErrSkillNotFound) { + t.Fatalf("unknown skill should fail with ErrSkillNotFound, got: %v", err) + } + msg := err.Error() + if !strings.HasPrefix(msg, "SKILL_NOT_FOUND") { + t.Errorf("error should lead with SKILL_NOT_FOUND, got: %s", msg) + } + for _, location := range []string{".opencode/skills/deploy/SKILL.md", ".opencode/skill/deploy/SKILL.md"} { + if !strings.Contains(msg, location) { + t.Errorf("error should name searched location %s, got: %s", location, msg) + } + } +} + +func TestResolveSkillRef_UnknownScriptPathFailsClosed(t *testing.T) { + root := t.TempDir() + + _, err := ResolveSkillRef(root, "scripts/missing.sh") + if !errors.Is(err, ErrSkillNotFound) { + t.Fatalf("unknown script should fail with ErrSkillNotFound, got: %v", err) + } + if msg := err.Error(); !strings.Contains(msg, "scripts/missing.sh") { + t.Errorf("error should name the missing path, got: %s", msg) + } +} + +func TestSpawnSubtask_ProjectPathDefaultsToCwd(t *testing.T) { + // The default project root is "."; a skill dir in the working + // directory resolves. t.Chdir resolves the default from within + // a temp dir and restores the working directory afterwards. + // (No test in this package uses t.Parallel.) + root := t.TempDir() + writeFile(t, root, ".opencode/skills/deploy/SKILL.md", "# deploy") + + t.Chdir(root) + + result, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + Skill: "deploy", + }) + if err != nil { + t.Fatalf("SpawnSubtask with default project_path: %v", err) + } + if result["skill"] != filepath.Join(".opencode", "skills", "deploy", "SKILL.md") { + t.Errorf("skill = %v, want resolved default-root path", result["skill"]) + } +} + +func TestSkillDirective_GoldenSkillName(t *testing.T) { + got := SkillDirective("deploy") + want := "Required skill: deploy\n\n" + + "This task is classified as deterministic work. Load the skill (invoke the `skill` tool with name \"deploy\") " + + "or run the script at deploy, and execute its full procedure. Do not substitute an improvised procedure." + if got != want { + t.Errorf("SkillDirective mismatch:\ngot: %q\nwant: %q", got, want) + } +} + +func TestSkillDirective_GoldenScriptPath(t *testing.T) { + got := SkillDirective("scripts/migrate.sh") + want := "Required skill: scripts/migrate.sh\n\n" + + "This task is classified as deterministic work. Load the skill (invoke the `skill` tool with name \"scripts/migrate.sh\") " + + "or run the script at scripts/migrate.sh, and execute its full procedure. Do not substitute an improvised procedure." + if got != want { + t.Errorf("SkillDirective mismatch:\ngot: %q\nwant: %q", got, want) + } +} + +func TestSpawnSubtask_EnvelopeRecordsDelegationFacts(t *testing.T) { + root := t.TempDir() + writeFile(t, root, ".opencode/skills/deploy/SKILL.md", "# deploy") + + result, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + Skill: "deploy", + ProjectPath: root, + }) + if err != nil { + t.Fatalf("SpawnSubtask: %v", err) + } + if result["task_classification"] != ClassificationDeterministic { + t.Errorf("task_classification = %v, want deterministic", result["task_classification"]) + } + wantPath := filepath.Join(".opencode", "skills", "deploy", "SKILL.md") + if result["skill"] != wantPath { + t.Errorf("skill = %v, want %v", result["skill"], wantPath) + } + if result["skill_directive"] != SkillDirective("deploy") { + t.Errorf("skill_directive = %v, want rendered directive", result["skill_directive"]) + } + if _, ok := result["skill_absent_reason"]; ok { + t.Error("envelope must not carry skill_absent_reason when absent") + } +} + +func TestSpawnSubtask_EnvelopeRecordsReasonVerbatim(t *testing.T) { + result, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + SkillAbsentReason: "no skill exists; procedure is one-off", + }) + if err != nil { + t.Fatalf("SpawnSubtask: %v", err) + } + if result["skill_absent_reason"] != "no skill exists; procedure is one-off" { + t.Errorf("skill_absent_reason = %v, want verbatim reason", result["skill_absent_reason"]) + } + if _, ok := result["skill"]; ok { + t.Error("envelope must not carry skill when only a reason is supplied") + } + if _, ok := result["skill_directive"]; ok { + t.Error("envelope must not carry skill_directive when no reference is supplied") + } +} + +func TestSpawnSubtask_EnvelopePreservesExistingFields(t *testing.T) { + result, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "Do thing", + Files: []string{"a.go"}, Description: "desc", SharedContext: "ctx", + TaskClassification: ClassificationJudgment, + }) + if err != nil { + t.Fatalf("SpawnSubtask: %v", err) + } + for _, key := range []string{"status", "bead_id", "epic_id", "title", "files", "description", "shared_context", "timestamp", "task_classification"} { + if _, ok := result[key]; !ok { + t.Errorf("envelope lost pre-existing key %q", key) + } + } + if result["task_classification"] != ClassificationJudgment { + t.Errorf("task_classification = %v, want judgment", result["task_classification"]) + } +} + +func TestSubtaskPrompt_RendersSkillDirective(t *testing.T) { + prompt := SubtaskPrompt( + "worker-1", "cell-abc", "epic-xyz", "Run migration", + []string{"migrate.go"}, "ctx", "scripts/migrate.sh", + ) + if !strings.Contains(prompt, "## Required Skill") { + t.Error("prompt should contain a Required Skill section") + } + if !strings.Contains(prompt, SkillDirective("scripts/migrate.sh")) { + t.Error("prompt should embed the fixed skill directive") + } +} + +func TestSubtaskPrompt_GoldenNoSkill(t *testing.T) { + got := SubtaskPrompt( + "worker-1", "cell-abc", "epic-xyz", "Implement auth", + []string{"auth.go", "auth_test.go"}, "shared ctx", "", + ) + // Byte-exact golden of the no-reference prompt shape; existing + // prompt consumers must see no diff when no skill is supplied. + want := "# Subtask: Implement auth\n\n" + + "**Agent:** worker-1\n" + + "**Cell ID:** cell-abc\n" + + "**Epic ID:** epic-xyz\n\n" + + "## Files to Modify\n\n" + + "- `auth.go`\n" + + "- `auth_test.go`\n" + + "\n" + + "## Shared Context\n\n" + + "shared ctx\n\n" + + "## Instructions\n\n" + + "1. Reserve files before editing using `comms_reserve`\n" + + "2. Implement the changes described above\n" + + "3. Write tests for new code\n" + + "4. Report progress with `forge_progress`\n" + + "5. Complete with `forge_complete` when done\n" + if got != want { + t.Errorf("no-skill prompt mismatch:\ngot: %q\nwant: %q", got, want) + } +} + +func TestSubtaskPrompt_GoldenWithSkill(t *testing.T) { + got := SubtaskPrompt( + "worker-1", "cell-abc", "epic-xyz", "Run migration", + []string{"migrate.go"}, "ctx", "scripts/migrate.sh", + ) + want := "# Subtask: Run migration\n\n" + + "**Agent:** worker-1\n" + + "**Cell ID:** cell-abc\n" + + "**Epic ID:** epic-xyz\n\n" + + "## Files to Modify\n\n" + + "- `migrate.go`\n" + + "\n" + + "## Shared Context\n\n" + + "ctx\n\n" + + "## Required Skill\n\n" + + "Required skill: scripts/migrate.sh\n\n" + + "This task is classified as deterministic work. Load the skill (invoke the `skill` tool with name \"scripts/migrate.sh\") " + + "or run the script at scripts/migrate.sh, and execute its full procedure. Do not substitute an improvised procedure.\n\n" + + "## Instructions\n\n" + + "1. Reserve files before editing using `comms_reserve`\n" + + "2. Implement the changes described above\n" + + "3. Write tests for new code\n" + + "4. Report progress with `forge_progress`\n" + + "5. Complete with `forge_complete` when done\n" + if got != want { + t.Errorf("with-skill prompt mismatch:\ngot: %q\nwant: %q", got, want) + } +} + func TestSubtaskPrompt(t *testing.T) { prompt := SubtaskPrompt( "worker-1", "cell-abc", "epic-xyz", "Implement auth", []string{"auth.go", "auth_test.go"}, - "Use JWT tokens", + "Use JWT tokens", "", ) checks := []string{ @@ -32,14 +381,18 @@ func TestSubtaskPrompt(t *testing.T) { } func TestSubtaskPrompt_NoFiles(t *testing.T) { - prompt := SubtaskPrompt("agent", "id", "epic", "Title", nil, "") + prompt := SubtaskPrompt("agent", "id", "epic", "Title", nil, "", "") if strings.Contains(prompt, "Files to Modify") { t.Error("should not show files section when empty") } } func TestSpawnSubtask(t *testing.T) { - result, err := SpawnSubtask("cell-1", "epic-1", "Do thing", []string{"a.go"}, "desc", "ctx") + result, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "Do thing", + Files: []string{"a.go"}, Description: "desc", SharedContext: "ctx", + TaskClassification: ClassificationJudgment, + }) if err != nil { t.Fatalf("SpawnSubtask: %v", err) } @@ -52,26 +405,199 @@ func TestSpawnSubtask(t *testing.T) { } func TestSpawnSubtask_MissingBeadID(t *testing.T) { - _, err := SpawnSubtask("", "epic-1", "title", nil, "", "") + _, err := SpawnSubtask(SpawnParams{EpicID: "epic-1", Title: "title"}) if err == nil { t.Error("expected error for missing bead_id") } } func TestSpawnSubtask_MissingEpicID(t *testing.T) { - _, err := SpawnSubtask("cell-1", "", "title", nil, "", "") + _, err := SpawnSubtask(SpawnParams{BeadID: "cell-1", Title: "title"}) if err == nil { t.Error("expected error for missing epic_id") } } func TestSpawnSubtask_MissingTitle(t *testing.T) { - _, err := SpawnSubtask("cell-1", "epic-1", "", nil, "", "") + _, err := SpawnSubtask(SpawnParams{BeadID: "cell-1", EpicID: "epic-1"}) if err == nil { t.Error("expected error for missing title") } } +func TestSpawnSubtask_ClassificationRequired(t *testing.T) { + _, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + }) + if err == nil { + t.Fatal("expected error for missing task_classification") + } + msg := err.Error() + if !strings.Contains(msg, "task_classification") { + t.Errorf("error should name task_classification, got: %s", msg) + } + if !strings.Contains(msg, "accepted values: deterministic, judgment") { + t.Errorf("error should list accepted values, got: %s", msg) + } +} + +func TestSpawnSubtask_ClassificationEnum(t *testing.T) { + for _, classification := range []string{"", "Deterministic", "urgent", "determinstic"} { + _, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: classification, + }) + if err == nil { + t.Errorf("classification %q should be rejected", classification) + continue + } + msg := err.Error() + if !strings.Contains(msg, "deterministic") || !strings.Contains(msg, "judgment") { + t.Errorf("error for %q should list accepted values, got: %s", classification, msg) + } + } +} + +func TestSpawnSubtask_JudgmentWithoutReferencePasses(t *testing.T) { + result, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationJudgment, + }) + if err != nil { + t.Fatalf("judgment without reference should pass: %v", err) + } + if result["status"] != "ready_to_spawn" { + t.Errorf("status = %v, want ready_to_spawn", result["status"]) + } +} + +func TestSpawnSubtask_MutuallyExclusiveFields(t *testing.T) { + _, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + Skill: "deploy", SkillAbsentReason: "no skill exists", + }) + if err == nil { + t.Fatal("skill + skill_absent_reason should be rejected") + } + if msg := err.Error(); !strings.Contains(msg, "mutually exclusive") { + t.Errorf("error should state mutual exclusivity, got: %s", msg) + } +} + +func TestSpawnSubtask_SkillWithJudgmentFails(t *testing.T) { + _, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationJudgment, + Skill: "deploy", + }) + if err == nil { + t.Fatal("skill with judgment classification should be rejected") + } + if msg := err.Error(); !strings.Contains(msg, "valid only for deterministic") { + t.Errorf("error should state skill is valid only for deterministic classification, got: %s", msg) + } +} + +func TestSpawnSubtask_ReasonWithJudgmentFails(t *testing.T) { + _, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationJudgment, + SkillAbsentReason: "no skill exists", + }) + if err == nil { + t.Fatal("skill_absent_reason with judgment classification should be rejected") + } + if msg := err.Error(); !strings.Contains(msg, "valid only for deterministic") { + t.Errorf("error should state skill_absent_reason is valid only for deterministic classification, got: %s", msg) + } +} + +func TestSpawnSubtask_DeterministicRequiresReferenceOrReason(t *testing.T) { + _, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + }) + if err == nil { + t.Fatal("deterministic without skill or skill_absent_reason should fail closed") + } + if !errors.Is(err, ErrSkillReferenceRequired) { + t.Fatalf("error should match ErrSkillReferenceRequired via errors.Is, got: %v", err) + } + msg := err.Error() + if !strings.HasPrefix(msg, "SKILL_REFERENCE_REQUIRED") { + t.Errorf("error message should lead with SKILL_REFERENCE_REQUIRED, got: %s", msg) + } + if !strings.Contains(msg, "skill_absent_reason") { + t.Errorf("error message should carry remediation naming skill_absent_reason, got: %s", msg) + } +} + +func TestSpawnSubtask_DeterministicWithReasonPasses(t *testing.T) { + result, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + SkillAbsentReason: "no skill exists for this task yet", + }) + if err != nil { + t.Fatalf("deterministic with skill_absent_reason should pass: %v", err) + } + if result["status"] != "ready_to_spawn" { + t.Errorf("status = %v, want ready_to_spawn", result["status"]) + } +} + +func TestSpawnSubtask_LengthBounds(t *testing.T) { + _, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + Skill: strings.Repeat("a", 257), + }) + if err == nil { + t.Fatal("skill over 256 chars should be rejected") + } + if msg := err.Error(); !strings.Contains(msg, "at most 256") { + t.Errorf("error should name the 256-char skill bound, got: %s", msg) + } + + _, err = SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + SkillAbsentReason: strings.Repeat("a", 513), + }) + if err == nil { + t.Fatal("skill_absent_reason over 512 chars should be rejected") + } + if msg := err.Error(); !strings.Contains(msg, "at most 512") { + t.Errorf("error should name the 512-char reason bound, got: %s", msg) + } +} + +func TestSpawnSubtask_LengthBoundaryValues(t *testing.T) { + // A skill of exactly the maximum length passes structural + // validation and fails only at resolution (the bound is inclusive). + _, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + Skill: strings.Repeat("a", MaxSkillRefLen), + }) + if !errors.Is(err, ErrSkillNotFound) { + t.Fatalf("boundary-length skill should pass validation and fail at resolution, got: %v", err) + } + + result, err := SpawnSubtask(SpawnParams{ + BeadID: "cell-1", EpicID: "epic-1", Title: "title", + TaskClassification: ClassificationDeterministic, + SkillAbsentReason: strings.Repeat("a", MaxSkillReasonLen), + }) + if err != nil { + t.Fatalf("boundary-length reason should be accepted: %v", err) + } + if result["status"] != "ready_to_spawn" { + t.Errorf("status = %v, want ready_to_spawn", result["status"]) + } +} + func TestCompleteSubtask(t *testing.T) { store := testStore(t) diff --git a/internal/tools/forge/tools.go b/internal/tools/forge/tools.go index ac439b0..1295885 100644 --- a/internal/tools/forge/tools.go +++ b/internal/tools/forge/tools.go @@ -3,6 +3,7 @@ package forge import ( "encoding/json" + "fmt" "github.com/unbound-force/replicator/internal/db" "github.com/unbound-force/replicator/internal/forge" @@ -203,7 +204,7 @@ func forgeValidateDecomposition() *registry.Tool { func forgeSubtaskPrompt() *registry.Tool { return ®istry.Tool{ Name: "forge_subtask_prompt", - Description: "Generate the prompt for a spawned subtask agent.", + Description: "Generate the prompt for a spawned subtask agent. When skill is supplied, the fixed skill directive is embedded in the prompt.", InputSchema: json.RawMessage(`{ "type": "object", "required": ["agent_name", "bead_id", "epic_id", "subtask_title", "files"], @@ -212,11 +213,12 @@ func forgeSubtaskPrompt() *registry.Tool { "bead_id": {"type": "string"}, "epic_id": {"type": "string"}, "subtask_title": {"type": "string"}, - "files": {"type": "array", "items": {"type": "string"}}, - "shared_context": {"type": "string"}, - "subtask_description": {"type": "string"} - } - }`), + "files": {"type": "array", "items": {"type": "string"}}, + "shared_context": {"type": "string"}, + "subtask_description": {"type": "string"}, + "skill": {"type": "string"} + } + }`), Execute: func(args json.RawMessage) (string, error) { var input struct { AgentName string `json:"agent_name"` @@ -225,11 +227,12 @@ func forgeSubtaskPrompt() *registry.Tool { SubtaskTitle string `json:"subtask_title"` Files []string `json:"files"` SharedCtx string `json:"shared_context"` + Skill string `json:"skill"` } if err := json.Unmarshal(args, &input); err != nil { return "", err } - prompt := forge.SubtaskPrompt(input.AgentName, input.BeadID, input.EpicID, input.SubtaskTitle, input.Files, input.SharedCtx) + prompt := forge.SubtaskPrompt(input.AgentName, input.BeadID, input.EpicID, input.SubtaskTitle, input.Files, input.SharedCtx, input.Skill) return prompt, nil }, } @@ -238,32 +241,53 @@ func forgeSubtaskPrompt() *registry.Tool { func forgeSpawnSubtask() *registry.Tool { return ®istry.Tool{ Name: "forge_spawn_subtask", - Description: "Prepare a subtask for spawning with Task tool.", + Description: "Prepare a subtask for spawning with Task tool. Delegation contract: task_classification is required (deterministic|judgment); deterministic-classified subtasks must supply a verified skill/script reference (skill) or a recorded skill_absent_reason (mutually exclusive); judgment-classified subtasks must omit both. Violations fail closed with SKILL_REFERENCE_REQUIRED or SKILL_NOT_FOUND.", InputSchema: json.RawMessage(`{ "type": "object", - "required": ["bead_id", "epic_id", "subtask_title", "files"], + "required": ["bead_id", "epic_id", "subtask_title", "files", "task_classification"], "properties": { "bead_id": {"type": "string"}, "epic_id": {"type": "string"}, "subtask_title": {"type": "string"}, "files": {"type": "array", "items": {"type": "string"}}, "subtask_description": {"type": "string"}, - "shared_context": {"type": "string"} + "shared_context": {"type": "string"}, + "task_classification": {"type": "string", "enum": ["deterministic", "judgment"]}, + "skill": {"type": "string", "minLength": 1, "maxLength": 256}, + "skill_absent_reason": {"type": "string", "minLength": 1, "maxLength": 512}, + "project_path": {"type": "string", "default": "."} } }`), Execute: func(args json.RawMessage) (string, error) { + // The *string shadows overlap the promoted SpawnParams + // string fields so encoding/json's shallowest-depth field + // resolution fills the pointers instead: nil means the + // field was omitted from the JSON, non-nil empty means it + // was supplied empty and is rejected below. SpawnSubtask + // itself treats empty strings as absent, so the tool + // boundary is the only layer that can tell them apart. var input struct { - BeadID string `json:"bead_id"` - EpicID string `json:"epic_id"` - Title string `json:"subtask_title"` - Files []string `json:"files"` - Description string `json:"subtask_description"` - SharedCtx string `json:"shared_context"` + forge.SpawnParams + Skill *string `json:"skill"` + SkillAbsentReason *string `json:"skill_absent_reason"` } if err := json.Unmarshal(args, &input); err != nil { return "", err } - result, err := forge.SpawnSubtask(input.BeadID, input.EpicID, input.Title, input.Files, input.Description, input.SharedCtx) + if input.Skill != nil && *input.Skill == "" { + return "", fmt.Errorf("skill must not be an empty string; omit the field when no skill applies") + } + if input.SkillAbsentReason != nil && *input.SkillAbsentReason == "" { + return "", fmt.Errorf("skill_absent_reason must not be an empty string; omit the field when no reason applies") + } + params := input.SpawnParams + if input.Skill != nil { + params.Skill = *input.Skill + } + if input.SkillAbsentReason != nil { + params.SkillAbsentReason = *input.SkillAbsentReason + } + result, err := forge.SpawnSubtask(params) if err != nil { return "", err } diff --git a/internal/tools/forge/tools_test.go b/internal/tools/forge/tools_test.go new file mode 100644 index 0000000..7fa8c89 --- /dev/null +++ b/internal/tools/forge/tools_test.go @@ -0,0 +1,219 @@ +package forge + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/unbound-force/replicator/internal/forge" + "github.com/unbound-force/replicator/internal/tools/registry" +) + +// schemaProperty returns the parsed properties map of a tool's input schema. +func schemaProperty(t *testing.T, tool *registry.Tool, name string) map[string]any { + t.Helper() + var schema struct { + Required []string `json:"required"` + Properties map[string]any `json:"properties"` + } + if err := json.Unmarshal(tool.InputSchema, &schema); err != nil { + t.Fatalf("unmarshal schema for %s: %v", tool.Name, err) + } + for _, req := range schema.Required { + if req == name { + prop, ok := schema.Properties[name].(map[string]any) + if !ok { + t.Fatalf("%s: property %q marked required but not defined", tool.Name, name) + } + return prop + } + } + t.Fatalf("%s: %q not in required list %v", tool.Name, name, schema.Required) + return nil +} + +func TestForgeSpawnSubtask_SchemaDocumentsContract(t *testing.T) { + tool := forgeSpawnSubtask() + + prop := schemaProperty(t, tool, "task_classification") + enum, ok := prop["enum"].([]any) + if !ok || len(enum) != 2 || enum[0] != "deterministic" || enum[1] != "judgment" { + t.Errorf("task_classification enum = %v, want [deterministic judgment]", prop["enum"]) + } + + var schema struct { + Required []string `json:"required"` + Properties map[string]map[string]any `json:"properties"` + } + if err := json.Unmarshal(tool.InputSchema, &schema); err != nil { + t.Fatalf("unmarshal schema: %v", err) + } + required := make(map[string]bool) + for _, name := range schema.Required { + required[name] = true + } + for _, name := range []string{"bead_id", "epic_id", "subtask_title", "files", "task_classification"} { + if !required[name] { + t.Errorf("schema should require %q (required: %v)", name, schema.Required) + } + } + for _, name := range []string{"skill", "skill_absent_reason", "project_path"} { + if _, ok := schema.Properties[name]; !ok { + t.Errorf("schema should document %q", name) + } + } + for _, name := range []string{"bead_id", "epic_id", "subtask_title", "files"} { + if _, ok := schema.Properties[name]; !ok { + t.Errorf("schema lost pre-existing property %q", name) + } + } + if skill := schema.Properties["skill"]; skill["minLength"] != float64(1) || skill["maxLength"] != float64(forge.MaxSkillRefLen) { + t.Errorf("skill bounds = %v/%v, want 1/%d", skill["minLength"], skill["maxLength"], forge.MaxSkillRefLen) + } + if reason := schema.Properties["skill_absent_reason"]; reason["minLength"] != float64(1) || reason["maxLength"] != float64(forge.MaxSkillReasonLen) { + t.Errorf("skill_absent_reason bounds = %v/%v, want 1/%d", reason["minLength"], reason["maxLength"], forge.MaxSkillReasonLen) + } + if def := schema.Properties["project_path"]["default"]; def != "." { + t.Errorf("project_path default = %v, want \".\"", def) + } +} + +func TestForgeSpawnSubtask_MissingClassificationRejected(t *testing.T) { + tool := forgeSpawnSubtask() + args := json.RawMessage(`{ + "bead_id": "cell-1", "epic_id": "epic-1", "subtask_title": "t", + "files": [] + }`) + _, err := tool.Execute(args) + if err == nil { + t.Fatal("omitted task_classification should be rejected") + } + msg := err.Error() + if !strings.Contains(msg, "task_classification") { + t.Errorf("error should name task_classification, got: %s", msg) + } + if !strings.Contains(msg, "accepted values: deterministic, judgment") { + t.Errorf("error should list accepted values, got: %s", msg) + } +} + +func TestForgeSpawnSubtask_EmptyStringSkillRejected(t *testing.T) { + tool := forgeSpawnSubtask() + args := json.RawMessage(`{ + "bead_id": "cell-1", "epic_id": "epic-1", "subtask_title": "t", + "files": [], "task_classification": "deterministic", "skill": "" + }`) + _, err := tool.Execute(args) + if err == nil { + t.Fatal("explicit empty skill should be rejected") + } + if msg := err.Error(); !strings.Contains(msg, "omit") { + t.Errorf("error should tell caller to omit the field, got: %s", msg) + } +} + +func TestForgeSpawnSubtask_EmptyStringReasonRejected(t *testing.T) { + tool := forgeSpawnSubtask() + args := json.RawMessage(`{ + "bead_id": "cell-1", "epic_id": "epic-1", "subtask_title": "t", + "files": [], "task_classification": "deterministic", "skill_absent_reason": "" + }`) + _, err := tool.Execute(args) + if err == nil { + t.Fatal("explicit empty skill_absent_reason should be rejected") + } + if msg := err.Error(); !strings.Contains(msg, "omit") { + t.Errorf("error should tell caller to omit the field, got: %s", msg) + } +} + +func TestForgeSpawnSubtask_ExecuteReturnsEnvelope(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, ".opencode", "skills", "deploy") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("# deploy"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + tool := forgeSpawnSubtask() + args := json.RawMessage(`{ + "bead_id": "cell-1", "epic_id": "epic-1", "subtask_title": "Run deploy", + "files": ["deploy.go"], "task_classification": "deterministic", + "skill": "deploy", "project_path": "` + root + `" + }`) + out, err := tool.Execute(args) + if err != nil { + t.Fatalf("Execute: %v", err) + } + + var envelope map[string]any + if err := json.Unmarshal([]byte(out), &envelope); err != nil { + t.Fatalf("unmarshal envelope: %v", err) + } + if envelope["task_classification"] != "deterministic" { + t.Errorf("task_classification = %v", envelope["task_classification"]) + } + if envelope["skill"] != filepath.Join(".opencode", "skills", "deploy", "SKILL.md") { + t.Errorf("skill = %v", envelope["skill"]) + } + if _, ok := envelope["skill_directive"]; !ok { + t.Error("envelope should carry skill_directive") + } +} + +func TestForgeSpawnSubtask_UnknownSkillFailsClosed(t *testing.T) { + // An empty temp root guarantees the skill is absent by + // construction rather than by machine-specific assumption. + root := t.TempDir() + tool := forgeSpawnSubtask() + args := json.RawMessage(`{ + "bead_id": "cell-1", "epic_id": "epic-1", "subtask_title": "t", + "files": [], "task_classification": "deterministic", + "skill": "does-not-exist", "project_path": "` + root + `" + }`) + _, err := tool.Execute(args) + if err == nil { + t.Fatal("unknown skill should fail closed") + } + if !strings.HasPrefix(err.Error(), "SKILL_NOT_FOUND") { + t.Errorf("error should lead with SKILL_NOT_FOUND, got: %s", err.Error()) + } +} + +func TestForgeSubtaskPrompt_SkillParamRendersDirective(t *testing.T) { + tool := forgeSubtaskPrompt() + args := json.RawMessage(`{ + "agent_name": "worker-1", "bead_id": "cell-1", "epic_id": "epic-1", + "subtask_title": "Run migration", "files": ["migrate.go"], + "skill": "scripts/migrate.sh" + }`) + out, err := tool.Execute(args) + if err != nil { + t.Fatalf("Execute: %v", err) + } + if !strings.Contains(out, "Required Skill") { + t.Error("prompt should contain the Required Skill section") + } + if !strings.Contains(out, "Do not substitute an improvised procedure.") { + t.Error("prompt should embed the fixed directive wording") + } +} + +func TestForgeSubtaskPrompt_NoSkillOmitsDirective(t *testing.T) { + tool := forgeSubtaskPrompt() + args := json.RawMessage(`{ + "agent_name": "worker-1", "bead_id": "cell-1", "epic_id": "epic-1", + "subtask_title": "Implement auth", "files": ["auth.go"] + }`) + out, err := tool.Execute(args) + if err != nil { + t.Fatalf("Execute: %v", err) + } + if strings.Contains(out, "Required Skill") { + t.Error("prompt without skill must not contain the directive section") + } +} diff --git a/openspec/changes/spawn-subtask-skill-enforcement/.openspec.yaml b/openspec/changes/spawn-subtask-skill-enforcement/.openspec.yaml new file mode 100644 index 0000000..d97497b --- /dev/null +++ b/openspec/changes/spawn-subtask-skill-enforcement/.openspec.yaml @@ -0,0 +1,2 @@ +schema: unbound-force +created: 2026-10-25 diff --git a/openspec/changes/spawn-subtask-skill-enforcement/design.md b/openspec/changes/spawn-subtask-skill-enforcement/design.md new file mode 100644 index 0000000..0826ce3 --- /dev/null +++ b/openspec/changes/spawn-subtask-skill-enforcement/design.md @@ -0,0 +1,245 @@ +## Context + +The parent change `delegation-skill-enforcement` +(unbound-force/unbound-force#787) closed the prose-only +posture on `uf_delegate_command` with a fail-closed parameter +contract. The Swarm parallel path was explicitly out of scope +there: `/uf.unleash` delegates `[P]` tasks through +`replicator_forge_spawn_subtask`, whose classification and +skill references live only in spawn-prompt prose (template +requirement plus divisor-guard review flagging). Issue #143 +tracks the Replicator-owned closure. + +Current state of the seam: + +- `forge.SpawnSubtask` (`internal/forge/spawn.go`) is a pure + function taking positional args (`beadID, epicID, title, + files, description, sharedContext`), validating only that + the IDs and title are non-empty. No filesystem access, no + project anchor. +- `forgeSpawnSubtask` (`internal/tools/forge/tools.go:238`) + registers the MCP tool with a documentation-only JSON + schema; the registry (`internal/tools/registry`) does not + auto-validate arguments -- validation happens inside each + tool's `Execute`. +- Tool errors surface through the MCP server + (`internal/mcp/server.go`) as `isError` responses with + `Error: ` text content, so greppable codes must + lead the error string. +- `forge.SubtaskPrompt` (exposed as `forge_subtask_prompt`) + composes the worker prompt; it is the Swarm path's + prompt-composition hook. +- No parity fixture covers `forge_spawn_subtask` (fixtures + exist only for `forge_decompose`, `forge_init`, + `forge_status`, `forge_worktree_list`), so additive envelope + fields are parity-safe. +- The embedded agent kit (`internal/agentkit/content/` -- + coordinator agent, `/forge` command, forge-coordination + skill) documents the old spawn call shape. + +Constitution alignment is assessed per principle in +proposal.md; this design is Principle III (Observable +Quality) applied to the spawn boundary, with Principle IV +shaping the test strategy. + +## Goals / Non-Goals + +### Goals + +- A `deterministic`-classified spawn without a verified + skill/script reference or a recorded absence reason fails + closed at the tool, before the subtask is marked ready to + spawn -- zero wasted child sessions. +- Every spawn declares its classification, closing the + silent-omission loophole (an unclassified spawn is + evadable-by-omission otherwise). +- Supplied references are existence-verified against the + project's skill directories or as repo-relative script + paths, with traversal attempts rejected. +- The classification, resolved reference, and absence reason + are durable, machine-parseable envelope fields. +- The skill directive rendered for the worker is + tool-produced (fixed wording), never agent-reconstructed. +- Judgment-classified spawns pass through unchanged -- zero + false positives by construction. + +### Non-Goals + +- No semantic classification inside the tool. Deciding + whether work is deterministic is judgment; the tool + validates the declared answer (parent D1, mirrored). +- No enforcement against user-level (`~/.config/opencode`) + skills. The rule binds delegated mechanical work to REPO + tooling. +- No embedding of the SKILL.md body into the composed prompt + -- a directive naming the skill, not the document. +- No skill registry or manifest; an existence check on two + known roots needs none. +- No schema-validation framework changes: the registry stays + documentation-schema-only; validation lives in the tool's + `Execute`, as every other tool does today. +- No modification of unbound-force templates in this repo -- + the `uf.unleash.md` update is a cross-repo issue filed from + this change's tasks. +- No change to the review/complete chain, comms, or org + layers. + +## Decisions + +### D1: The caller classifies; the tool enforces + +`task_classification` is REQUIRED on every +`forge_spawn_subtask` call. Every spawn is a freeform task +delegation -- the direct analog of the parent's prompt-only +delegation, for which classification is mandatory. Making it +optional here would recreate the exact loophole this change +closes: skip classification, skip the rule. + +Structural rules validated before any filesystem work: + +- enum membership (only `deterministic` and `judgment`) +- `skill` and `skill_absent_reason` mutually exclusive +- either field valid only with `deterministic` classification +- length bounds (`skill` 1-256, `skill_absent_reason` + 1-512); empty strings are invalid input -- absence is + expressed by omitting the field + +Structural failures return plain input-validation errors in +the existing house style (`task_classification is required +(accepted values: deterministic, judgment)`). The two rules that +need dedicated codes run next and return sentinel-based +envelopes (D2). + +Alternatives rejected: + +- **Optional classification, rules fire only when + deterministic is supplied.** Evadable by omission; recreates + the prose-only posture. +- **Heuristic skill detection in prompt text.** The tool + cannot reliably recognize a reference in free text (parent + D1, mirrored). + +### D2: Fail-closed rules are sentinel errors with greppable codes + +`internal/forge` defines two sentinel errors, wrapped with +call context but `errors.Is`-matchable: + +- `ErrSkillReferenceRequired` -- + `SKILL_REFERENCE_REQUIRED: ...` with remediation (supply a + skill/script reference or record why none applies) +- `ErrSkillNotFound` -- `SKILL_NOT_FOUND: ...` naming the + searched locations + +The MCP server renders tool errors as `Error: ` text +in an `isError` response, so the code leads the error string +and is greppable in logs and transcripts. This follows the +remediation-carrying error-contract precedent +(`DEWEY_UNAVAILABLE`, parent D4/D5) and the repo convention +of sentinel errors over string comparison. + +Validation order: structural rules, then +deterministic-requires-reference-or-reason, then existence +verification. A rejected call costs one tool round-trip, zero +spawned agents. + +### D3: Skill resolution is an existence check on two roots plus a traversal guard + +New `forge.ResolveSkillRef(projectPath, ref)` resolves a +reference against the project root: + +- No `/` in `ref`: skill name -- check + `.opencode/skills//SKILL.md`, then + `.opencode/skill//SKILL.md` (both roots exist across + org repos; a name in both resolves to the first root). +- `/` in `ref`: repo-relative path -- reject absolute paths + and any `..` path element after cleaning, then existence- + check the path. + +Failures return `ErrSkillNotFound` naming the searched +locations. Resolution uses `os.Stat` only -- no registry, no +manifest, no new dependencies. Tests use `t.TempDir()` with +synthetic skill roots and script files (Constitution IV). + +Alternatives rejected: + +- **Skill registry/manifest** -- adds a drift surface for + zero need (parent D2, mirrored). +- **Searching user-level skill directories** -- the rule + binds delegated mechanical work to repo tooling. + +### D4: The directive is a tool-rendered constant + +A fixed `skillDirective(reference)` renders the worker +directive ("load the skill / run the script and execute its +full procedure; do not substitute an improvised procedure"). +It appears in exactly two places, both tool-produced: + +- the `forge_spawn_subtask` success envelope as + `skill_directive` +- the `forge_subtask_prompt` composed prompt, via a new + optional `skill` parameter on that tool + +The wording is a design constant covered by golden-string +tests. The coordinator never reconstructs it from prose. + +### D5: The envelope is additive; the signature becomes a struct + +`SpawnSubtask` grows from six positional parameters to ten +inputs. Rather than a ten-arg call, the function takes a +`SpawnParams` struct (`internal/forge`), and the tool layer +unmarshals JSON into it. The success envelope keeps every +existing field name and shape, adding `task_classification`, +the resolved `skill` path, `skill_absent_reason` (verbatim), +and `skill_directive` (when a reference was supplied). +Existing internal callers are updated in the same change; +`SpawnSubtask` is not referenced outside this repo. + +`project_path` is optional with default `.` -- consistent with +the `project_path` convention of comms and worktree tools -- + +and is only consulted when a reference needs resolution, so +judgment-classified spawns need no filesystem anchor. + +### D6: Embedded agent kit and docs move with the contract + +The coordinator agent, `/forge` command, and +forge-coordination skill show `forge_spawn_subtask` call +shapes; each is updated to carry `task_classification` and +the reference-or-reason rule (agent-kit changes are +spec-gated -- this change is that spec). `docs/tools.md` is +regenerated via `replicator docs`. + +Cross-repo: an issue is filed in unbound-force/unbound-force +to move the `uf.unleash.md` Swarm-path prose requirement into +tool parameters, closing the loop the parent change opened in +its task 5.5. Replicator's error remediation text names the +missing parameter and accepted values, so a version-skewed +caller fails self-describingly rather than confusingly. + +## Risks / Trade-offs + +- **Breaking change for version-skewed callers.** Hero-repo + templates that omit `task_classification` fail closed until + the unbound-force template update lands. Mitigation: + remediation-carrying error text, a tracked cross-repo + issue, and internal mirrors updated in the same change. + Accepted -- this is the requirement working as intended + (parent precedent: "future delegating commands must + classify"). +- **Misclassification evades the tool.** A caller can label + judgment work to skip the requirement. The classification + is now an explicit, recorded envelope field the guard layer + audits against the task text; the prior alternative was no + claim at all. Layered mitigation, accepted. +- **Escape-hatch abuse.** `skill_absent_reason` could become + a free pass. It is recorded verbatim in durable state and + is the first thing a reviewer reads; vacuous reasons are + flaggable findings at the guard layer (already in place per + the parent change). Accepted. +- **Skill-root hardcoding.** The two roots are org + convention; the resolution order is a documented constant. + A future layout change is a one-constant edit. +- **Prompt-regression surface.** `SubtaskPrompt` gains a + conditional section; golden-string tests pin both the + with-directive and without-directive shapes so existing + prompt consumers see no diff when no skill is supplied. diff --git a/openspec/changes/spawn-subtask-skill-enforcement/proposal.md b/openspec/changes/spawn-subtask-skill-enforcement/proposal.md new file mode 100644 index 0000000..ba83c76 --- /dev/null +++ b/openspec/changes/spawn-subtask-skill-enforcement/proposal.md @@ -0,0 +1,222 @@ +## Why + +The parent change `delegation-skill-enforcement` +(unbound-force/unbound-force#787) extended `uf_delegate_command` +with a delegation parameter contract -- +`task_classification` (`deterministic` | `judgment`), `skill` +(existence-verified skill name or repo-relative script path), +and `skill_absent_reason` -- enforced fail-closed at the tool +boundary, before any child session spawns. + +The `/uf.unleash` Swarm parallel path delegates `[P]` tasks +through `replicator_forge_spawn_subtask`. On that path, +classification and skill references live only inside the +spawn-prompt prose: a template-level requirement in the +command file plus divisor-guard review flagging. Nothing at +the tool level validates them. This is the exact prose-only +posture the parent change closed on `uf_delegate_command` -- +agent compliance with no detection when a spawn skips the +reference (post-compaction drift, a hurried re-run, or a +future command repeating the defect). + +Tool-level validation on the Swarm path is Replicator-owned +and was recorded as a design Non-Goal in the parent change; +this change is its tracked closure +([unbound-force/replicator#143](https://github.com/unbound-force/replicator/issues/143)). + +## What Changes + +Add equivalent parameter-level validation to +`forge_spawn_subtask`, mirroring `uf_delegate_command`'s +contract: + +1. **New parameters** on the `forge_spawn_subtask` MCP tool: + - `task_classification` (`deterministic` | `judgment`) -- + REQUIRED on every call. Every spawn is a freeform task + delegation (the analog of the parent's prompt-only + delegation), so requiring classification closes the + silent-omission loophole: an unclassified spawn fails + with input validation, not a silent pass-through. + - `skill` (min 1, max 256 chars) -- a skill name resolved + against the project's skill directories + (`.opencode/skills//SKILL.md`, then + `.opencode/skill//SKILL.md`), or a repo-relative + script path (any value containing `/`), existence- + verified before the subtask is marked ready to spawn. + - `skill_absent_reason` (min 1, max 512 chars) -- the + recorded justification when deterministic work has no + applicable repo skill or script. + - `project_path` (optional, defaults to `.`) -- the + filesystem anchor for reference resolution, matching the + `project_path` convention used by comms and worktree + tools. + +2. **Fail-closed validation rules** (all evaluated before the + success envelope is returned): + - `deterministic` classification with no `skill` and no + `skill_absent_reason` fails with + `SKILL_REFERENCE_REQUIRED`. + - An unresolvable `skill` reference fails with + `SKILL_NOT_FOUND`, naming the searched locations. + - `skill` and `skill_absent_reason` are mutually + exclusive. + - Either field supplied with `judgment` classification + fails input validation (the rendered directive asserts + deterministic work). + - `task_classification` values outside the enum fail input + validation; empty strings are invalid input for all three + fields (absence is expressed by omitting the field). + +3. **Tool-rendered skill directive**: the spawn result + envelope gains a `skill_directive` field -- a fixed, + tool-rendered block instructing the worker to load the + skill or run the script, never agent-reconstructed. + `forge_subtask_prompt` gains an optional `skill` parameter + that renders the same fixed directive into the composed + worker prompt. + +4. **Audit record**: the spawn result envelope gains + `task_classification`, `skill` (the resolved reference), + and `skill_absent_reason` (when present) -- additive, + machine-parseable fields reviewers audit without prompt + archaeology. + +5. **Embedded agent-kit call shapes updated**: the + coordinator agent, `/forge` command, and forge-coordination + skill documents that demonstrate + `forge_spawn_subtask(...)` calls are updated to carry the + new parameters. + +6. **Cross-repo coordination**: file an issue in + unbound-force/unbound-force to update the `uf.unleash.md` + Swarm-path spawn calls to pass the new parameters (the + template already requires classification + skill reference + in prose per the parent change -- this moves them into + parameters). + +## Capabilities + +### New Capabilities + +- `spawn-delegation-contract`: parameter-level task + classification and skill/script reference validation on the + Swarm spawn path, fail-closed before a subtask is marked + ready to spawn, with a durable audit record in the spawn + result envelope. + +### Modified Capabilities + +- `forge_spawn_subtask`: input contract extended with + `task_classification` (required), `skill`, + `skill_absent_reason`, and `project_path`; result envelope + extended with classification, resolved reference, absence + reason, and the tool-rendered `skill_directive`. BREAKING + for callers that omit `task_classification`. +- `forge_subtask_prompt`: optional `skill` parameter renders + the fixed skill directive into the composed worker prompt. +- Embedded agent kit (coordinator agent, `/forge` command, + forge-coordination skill): documented spawn call shapes + carry the new parameters. + +### Removed Capabilities + +- None. + +## Impact + +- **Files**: + - `internal/forge/spawn.go` -- validation, reference + resolution, directive rendering, sentinel errors + - `internal/tools/forge/tools.go` -- tool schemas and + parameter plumbing for `forge_spawn_subtask` and + `forge_subtask_prompt` + - `internal/forge/spawn_test.go` -- unit coverage for every + validation rule and resolution path + - `internal/tools/forge/tools_test.go` -- tool-layer schema, + envelope, and fail-closed coverage + - `internal/agentkit/content/` -- coordinator agent, forge + command, forge-coordination skill call-shape updates + - `internal/agentkit/agentkit_test.go` -- contract marker test + pinning the delegation contract in the embedded kit + - `.opencode/skills/forge-coordination/SKILL.md` and + `.opencode/commands/forge.md` -- repo-level mirrors kept in + sync with the embedded kit + - `docs/tools.md` -- regenerated via `replicator docs` +- **Breaking change**: existing callers that omit + `task_classification` fail closed with a remediation-carrying + error. Internal agent-kit call shapes are updated in the + same change; hero-repo templates (`uf.unleash.md`) are + updated through the cross-repo coordination issue. The + failure message names the missing parameter and accepted + values, so a version-skewed caller is self-describing. +- **No semantic classification**: the tool validates the + declared answer; deciding whether work is deterministic + remains the delegating agent's judgment (parent design D1). +- **No enforcement against user-level skills**: the rule + binds delegated mechanical work to REPO tooling; + `~/.config/opencode` skills are out of scope (parent + Non-Goal, mirrored). +- **Parity**: `forge_spawn_subtask` has no TypeScript fixture + in `test/parity/fixtures/` (only `forge_decompose`, + `forge_init`, `forge_status`, `forge_worktree_list` are + covered), so the additive envelope fields introduce no + parity break. +- **Risk**: misclassification evasion (labeling judgment work + to skip the requirement) is an explicit, recorded claim the + guard layer already audits against the task text -- the + same layered-mitigation posture the parent accepted. + +## Constitution Alignment + +Assessed against the Replicator constitution +(`.specify/memory/constitution.md`), which extends the +Unbound Force org constitution v1.1.0. + +### I. Autonomous Collaboration + +**Assessment**: PASS + +The contract moves a coordination rule from prose (template +requirement + review flagging) into the tool boundary, making +the spawn result a self-describing artifact: classification, +resolved reference, absence reason, and the rendered directive +are machine-parseable fields any consumer can audit without +prompt archaeology. The tool remains callable independently; +no runtime coupling is introduced. + +### II. Composability First + +**Assessment**: PASS + +The binary remains standalone: reference resolution is a pure +filesystem existence check against the project's own skill +directories, with no registry, manifest, or external service. +Skill resolution degrades to a structured, greppable +`SKILL_NOT_FOUND` error rather than crashing. No new +dependencies. The required-classification breaking change is +coordinated with the unbound-force template update through a +tracked cross-repo issue, mirroring the parent change's +downstream-lag mitigation. + +### III. Observable Quality + +**Assessment**: PASS + +Every validation failure is a structured, greppable error +code (`SKILL_REFERENCE_REQUIRED`, `SKILL_NOT_FOUND`) with +remediation text, surfaced through the existing MCP +error-content mechanism. The success envelope records the +classification, resolved reference, and justification -- +durable, machine-parseable provenance for the spawn decision. +The fixed directive wording is covered by golden-string tests. + +### IV. Testability + +**Assessment**: PASS + +Validation rules are pure-function unit tests; reference +resolution uses `t.TempDir()` with synthetic +`.opencode/skills//SKILL.md` and script-file fixtures -- +no external services, no network, no shared state. Every rule +in this proposal has a corresponding test scenario in the +specification delta. diff --git a/openspec/changes/spawn-subtask-skill-enforcement/specs/spawn-delegation-contract/spec.md b/openspec/changes/spawn-subtask-skill-enforcement/specs/spawn-delegation-contract/spec.md new file mode 100644 index 0000000..5fbb28d --- /dev/null +++ b/openspec/changes/spawn-subtask-skill-enforcement/specs/spawn-delegation-contract/spec.md @@ -0,0 +1,162 @@ +## ADDED Requirements + +### Requirement: Spawn task classification is declared and validated + +The `forge_spawn_subtask` tool MUST require a `task_classification` parameter on every call. The value MUST be exactly `deterministic` or `judgment`. A call that omits `task_classification`, supplies an empty string, or supplies any other value MUST fail input validation before any other processing, with an error message naming the parameter and the accepted values. The tool MUST NOT attempt to classify the task itself -- it validates the declared answer only. + +#### Scenario: Omitted classification fails + +- **GIVEN** a caller invokes `forge_spawn_subtask` with valid `bead_id`, `epic_id`, `subtask_title`, and `files`, but no `task_classification` +- **WHEN** the call is executed +- **THEN** the tool MUST return an input validation error naming `task_classification` as required and listing `deterministic` and `judgment` as the accepted values +- **AND** the subtask MUST NOT be marked ready to spawn + +#### Scenario: Non-enum classification fails + +- **GIVEN** a caller supplies `task_classification` with a value other than `deterministic` or `judgment` +- **WHEN** the call is executed +- **THEN** the tool MUST return an input validation error listing the accepted values + +#### Scenario: Judgment-classified spawn passes without reference + +- **GIVEN** a caller supplies `task_classification` of `judgment` with no `skill` and no `skill_absent_reason` +- **WHEN** the call is executed with otherwise valid arguments +- **THEN** the tool MUST return a success envelope with `task_classification` recorded as `judgment` + +### Requirement: Deterministic spawns require a reference or a recorded reason + +A `deterministic`-classified spawn MUST supply either a `skill` reference or a `skill_absent_reason`. A deterministic-classified call with neither MUST fail closed with a `SKILL_REFERENCE_REQUIRED` error that carries remediation text (supply a skill/script reference, or record why none applies). The check MUST run before the subtask is marked ready to spawn. + +#### Scenario: Deterministic without reference or reason fails closed + +- **GIVEN** a caller supplies `task_classification` of `deterministic` with no `skill` and no `skill_absent_reason` +- **WHEN** the call is executed +- **THEN** the tool MUST return an error containing the code `SKILL_REFERENCE_REQUIRED` +- **AND** the error message MUST state the remediation (supply a skill or skill_absent_reason) +- **AND** the subtask MUST NOT be marked ready to spawn + +#### Scenario: Deterministic with absence reason passes + +- **GIVEN** a caller supplies `task_classification` of `deterministic` and a non-empty `skill_absent_reason`, with no `skill` +- **WHEN** the call is executed with otherwise valid arguments +- **THEN** the tool MUST return a success envelope recording the `skill_absent_reason` verbatim + +### Requirement: Supplied references are existence-verified before spawn + +When `skill` is supplied, the tool MUST verify the reference exists on disk before returning a success envelope, resolving against the project root given by `project_path` (default `.`). Resolution rules are defined by the skill-name and repo-relative-path requirements below. An unresolvable or rejected reference MUST fail closed with a `SKILL_NOT_FOUND` error naming the searched locations. + +#### Scenario: Unknown reference fails closed + +- **GIVEN** a project with no skill or script matching the supplied `skill` value +- **WHEN** the call is executed +- **THEN** the tool MUST return an error containing the code `SKILL_NOT_FOUND` that names every searched location +- **AND** the subtask MUST NOT be marked ready to spawn + +### Requirement: Skill names resolve against the project skill directories + +A `skill` value without `/` MUST resolve as a skill name: `.opencode/skills//SKILL.md` first, then `.opencode/skill//SKILL.md`. A name present in both roots MUST resolve to the first root. + +#### Scenario: Skill name resolves through the primary root + +- **GIVEN** a project whose `.opencode/skills/deploy/SKILL.md` exists +- **WHEN** a deterministic-classified spawn supplies `skill` of `deploy` +- **THEN** the tool MUST resolve the reference to `.opencode/skills/deploy/SKILL.md` and return a success envelope carrying the resolved path + +#### Scenario: Skill name falls back to the secondary root + +- **GIVEN** a project where `.opencode/skills//SKILL.md` does not exist but `.opencode/skill//SKILL.md` does +- **WHEN** a spawn supplies `skill` of `` +- **THEN** the tool MUST resolve the reference to `.opencode/skill//SKILL.md` + +### Requirement: Repo-relative script paths resolve with a traversal guard + +A `skill` value containing `/` MUST resolve as a repo-relative path. Absolute paths and paths containing a `..` path element remaining after path cleaning MUST be rejected as traversal attempts. + +#### Scenario: Repo-relative script path resolves + +- **GIVEN** a project containing `scripts/lint.sh` +- **WHEN** a spawn supplies `skill` of `scripts/lint.sh` +- **THEN** the tool MUST resolve the reference to `scripts/lint.sh` and verify its existence + +#### Scenario: Traversal attempts are rejected + +- **GIVEN** a spawn supplying `skill` of `../secrets/env.sh` or an absolute path +- **WHEN** the call is executed +- **THEN** the tool MUST return a `SKILL_NOT_FOUND` error and MUST NOT touch any path outside the project root + +### Requirement: Reference and reason are mutually exclusive and deterministic-only + +`skill` and `skill_absent_reason` MUST NOT be supplied together. Either field supplied with a `judgment` classification MUST fail input validation, because the rendered directive asserts deterministic work. Empty strings MUST be invalid input for `skill` (bounds 1-256 chars) and `skill_absent_reason` (bounds 1-512 chars); absence MUST be expressed by omitting the field. + +#### Scenario: Both reference and reason fail + +- **GIVEN** a deterministic-classified spawn supplying both `skill` and `skill_absent_reason` +- **WHEN** the call is executed +- **THEN** the tool MUST return an input validation error stating the fields are mutually exclusive + +#### Scenario: Reference with judgment classification fails + +- **GIVEN** a spawn supplying `task_classification` of `judgment` and a `skill` value +- **WHEN** the call is executed +- **THEN** the tool MUST return an input validation error stating `skill` is valid only with `deterministic` classification + +#### Scenario: Empty-string fields are invalid + +- **GIVEN** a spawn supplying `skill` or `skill_absent_reason` as an empty string +- **WHEN** the call is executed +- **THEN** the tool MUST return an input validation error stating that absence is expressed by omitting the field + +### Requirement: Spawn envelope records the delegation facts + +On success, the `forge_spawn_subtask` result envelope MUST include `task_classification`; the resolved `skill` reference (the path resolved on disk) when one was supplied; and `skill_absent_reason` verbatim when one was supplied. These fields are additive -- existing envelope fields MUST retain their names and shapes. + +#### Scenario: Envelope carries classification and resolved reference + +- **GIVEN** a deterministic-classified spawn with a verified `skill` reference +- **WHEN** the tool returns its success envelope +- **THEN** the envelope MUST contain `task_classification` of `deterministic`, the resolved skill path, and no `skill_absent_reason` field + +#### Scenario: Envelope carries absence reason + +- **GIVEN** a deterministic-classified spawn with a `skill_absent_reason` +- **WHEN** the tool returns its success envelope +- **THEN** the envelope MUST contain the reason verbatim and no resolved skill path + +### Requirement: Skill directive is tool-rendered + +When a verified `skill` reference exists, the tool MUST render a fixed skill-directive block instructing the worker to load the named skill or run the referenced script and follow its full procedure without substitution. The directive MUST be produced by the tool, not reconstructed by the calling agent. + +#### Scenario: Envelope carries the rendered directive + +- **GIVEN** a deterministic-classified spawn with a verified `skill` reference +- **WHEN** the tool returns its success envelope +- **THEN** the envelope MUST contain a `skill_directive` field whose text names the skill or script and instructs the worker to execute its full procedure + +#### Scenario: Subtask prompt embeds the directive + +- **GIVEN** a `forge_subtask_prompt` call supplying a `skill` reference +- **WHEN** the prompt is composed +- **THEN** the composed prompt MUST contain the same fixed directive block +- **AND** a spawn without a skill reference MUST NOT contain the directive block + +### Requirement: forge_spawn_subtask input contract + +The `forge_spawn_subtask` tool input contract MUST accept the existing parameters (`bead_id`, `epic_id`, `subtask_title`, `files`, `subtask_description`, `shared_context`) plus `task_classification` (required), `skill`, `skill_absent_reason`, and `project_path` (optional, default `.`). The JSON schema MUST document the enum, bounds, and required status of each new parameter. + +#### Scenario: Schema documents the new contract + +- **GIVEN** the registered `forge_spawn_subtask` tool +- **WHEN** its input schema is inspected +- **THEN** the schema MUST list `task_classification` as required with enum values `deterministic` and `judgment` +- **AND** MUST document `skill`, `skill_absent_reason`, and `project_path` +- **AND** the previously required inputs (`bead_id`, `epic_id`, `subtask_title`, `files`) MUST remain required with unchanged types + +### Requirement: Embedded agent kit demonstrates the parameter contract + +The embedded coordinator agent definition, `/forge` command, and forge-coordination skill MUST show `forge_spawn_subtask` call shapes that include `task_classification` and, for deterministic work, a `skill` reference or `skill_absent_reason`. Previously the documented call shapes were `forge_spawn_subtask(bead_id, epic_id, subtask_title, files)` with no classification parameters. + +#### Scenario: Coordinator protocol carries the contract + +- **GIVEN** the embedded coordinator agent or forge command content +- **WHEN** the spawn step is read +- **THEN** the documented call MUST include `task_classification` and the reference-or-reason rule diff --git a/openspec/changes/spawn-subtask-skill-enforcement/tasks.md b/openspec/changes/spawn-subtask-skill-enforcement/tasks.md new file mode 100644 index 0000000..cf75727 --- /dev/null +++ b/openspec/changes/spawn-subtask-skill-enforcement/tasks.md @@ -0,0 +1,142 @@ + + +## 1. Validation core (internal/forge) + +All tasks in this group modify `internal/forge/spawn.go` and +`internal/forge/spawn_test.go` (or add a sibling test file in +the same package) -- no parallel execution. Follow +red-green-refactor: write the failing test first, then the +minimal implementation. + +- [x] 1.1 Introduce the `SpawnParams` struct (bead_id, epic_id, + subtask_title, files, subtask_description, shared_context, + task_classification, skill, skill_absent_reason, + project_path) and refactor `SpawnSubtask` to take it. + Port the existing `TestSpawnSubtask*` tests to the struct + call shape; existing required-field behavior (missing + bead_id/epic_id/title fails) stays green. (design decision + D5) + **Files**: `internal/forge/spawn.go`, + `internal/forge/spawn_test.go` +- [x] 1.2 Structural validation with tests: + `task_classification` required and enum-checked + (`deterministic` | `judgment`); `skill` and + `skill_absent_reason` mutually exclusive; either field + rejected with `judgment` classification; length bounds + (`skill` 1-256, `skill_absent_reason` 1-512) with empty + strings invalid. Error text names the parameter and + accepted values. (design decision D1) + **Files**: `internal/forge/spawn.go`, + `internal/forge/spawn_test.go` +- [x] 1.3 Deterministic-requires-reference-or-reason with + tests: a `deterministic`-classified call with neither + `skill` nor `skill_absent_reason` fails closed via the + `ErrSkillReferenceRequired` sentinel whose message leads + with `SKILL_REFERENCE_REQUIRED` and carries remediation + text; a deterministic call with a reason (no reference) + passes. Verify sentinel matching with `errors.Is` through + wrapping. (design decision D2) + **Files**: `internal/forge/spawn.go`, + `internal/forge/spawn_test.go` +- [x] 1.4 `ResolveSkillRef(projectPath, ref)` with tests using + `t.TempDir()`: skill name resolves + `.opencode/skills//SKILL.md` then + `.opencode/skill//SKILL.md` (first root wins when both + exist); `/`-containing values resolve as repo-relative + paths; absolute paths and `..` elements are rejected; + unresolvable references fail via `ErrSkillNotFound` + (`SKILL_NOT_FOUND`, naming searched locations). Wire + resolution into `SpawnSubtask` with `project_path` + defaulting to `.`. (design decisions D3, D5) + **Files**: `internal/forge/spawn.go`, + `internal/forge/spawn_test.go` +- [x] 1.5 Directive and audit envelope with tests: fixed + `skillDirective` rendering (golden-string test); success + envelope gains `task_classification`, resolved `skill` + path, `skill_absent_reason` (verbatim, when present), and + `skill_directive` (when a reference was supplied); all + pre-existing envelope fields keep their names and shapes; + `SubtaskPrompt` renders the same directive when given a + reference and produces byte-identical output to today when + no reference is given. (design decisions D4, D5) + **Files**: `internal/forge/spawn.go`, + `internal/forge/spawn_test.go` + +## 2. Tool layer (internal/tools/forge) + +Same file throughout -- no parallel execution. + +- [x] 2.1 Update the `forge_spawn_subtask` tool: input schema + documents `task_classification` (required, enum), `skill`, + `skill_absent_reason`, and `project_path`; `Execute` + unmarshals into `forge.SpawnParams` and returns the + enriched envelope. (design decision D5) + **Files**: `internal/tools/forge/tools.go`, + `internal/tools/forge/tools_test.go` +- [x] 2.2 Add the optional `skill` parameter to the + `forge_subtask_prompt` tool (schema + prompt composition via + `SubtaskPrompt`). (design decision D4) + **Files**: `internal/tools/forge/tools.go`, + `internal/tools/forge/tools_test.go` + +## 3. Embedded agent kit and docs + +Different files -- parallel-eligible after groups 1-2 land +(the documented call shapes must reflect the final parameter +names). + +- [x] 3.1 [P] Update the coordinator agent definition's spawn + step to show `task_classification` and the + reference-or-reason rule. + **Files**: `internal/agentkit/content/agents/coordinator.md` +- [x] 3.2 [P] Update the `/forge` command workflow's spawn + step (step 5) with the same contract. + **Files**: `internal/agentkit/content/commands/forge.md`, + `.opencode/commands/forge.md` (repo-level mirror synced) +- [x] 3.3 [P] Update the forge-coordination skill's + Coordinator Protocol spawn step with the same contract. + **Files**: + `internal/agentkit/content/skills/forge-coordination/SKILL.md`, + `.opencode/skills/forge-coordination/SKILL.md` (kept + byte-identical to the embedded copy per the drift test), + `internal/agentkit/agentkit_test.go` (contract marker test) +- [x] 3.4 Regenerate the MCP tool reference from the updated + registration: `go run ./cmd/replicator docs` (verify the + diff shows only the two changed tools). + **Files**: `docs/tools.md` + +## 4. Governance and verification + +- [x] 4.1 File the cross-repo issue in unbound-force/unbound-force + to update the `uf.unleash.md` Swarm-path spawn calls to pass + `task_classification` / `skill` / `skill_absent_reason` as + tool parameters (closes the loop opened by the parent + change's task 5.5; link this change and issue #143). Do not + modify unbound-force files from this repo. (design decision + D6) + Filed as [unbound-force/unbound-force#830](https://github.com/unbound-force/unbound-force/issues/830). +- [x] 4.2 Website documentation gate: file an issue in + unbound-force/website covering the changed + `forge_spawn_subtask` / `forge_subtask_prompt` contracts + (new required parameter, failure codes, envelope fields). + Filed as [unbound-force/website#329](https://github.com/unbound-force/website/issues/329). +- [x] 4.3 CI parity gate: run `make check` (vet + test) and + `make check-coverage`; confirm zero failures and no + coverage-ratchet regression. Confirm `gofmt`/`goimports` + cleanliness on touched files. +- [x] 4.4 Constitution alignment verification: confirm every + spec scenario in + `specs/spawn-delegation-contract/spec.md` has a passing test + (Principle IV), error strings lead with greppable codes + (Principle III), and no new dependency or external service + entered the build (Principle II).