diff --git a/forge-cli/build/dockerfile_stage.go b/forge-cli/build/dockerfile_stage.go index e61f344..1066069 100644 --- a/forge-cli/build/dockerfile_stage.go +++ b/forge-cli/build/dockerfile_stage.go @@ -295,12 +295,39 @@ func copyDir(src, dst string) error { } func (s *DockerfileStage) writeDockerignore(bc *pipeline.BuildContext) error { - dockerignoreContent := `.env + // Two categories of exclusions: + // 1. Secrets — never bake credentials into a layered image. + // 2. Operator-side artifacts the runtime does not open. The + // Dockerfile uses `COPY . .` from .forge-output/, so without + // these every operator-side helper lands in /app/, padding + // the image and leaking build metadata. The runtime opens + // only forge.yaml, guardrails.json, -config.yaml, + // agent.json, policy-scaffold.json, skills/, and + // checksums.json — anything outside that set is excluded. + dockerignoreContent := `# --- Secrets --- +.env .env.* *.enc secrets.enc *.key *.pem + +# --- Operator-side artifacts (issue #147) --- +# k8s/ is for kubectl apply on the host, not the runtime image. +k8s/ +# build-manifest.json is consumed by forge package / forge export. +build-manifest.json +# compiled/ holds the egress allowlist (used by forge package and +# the K8s NetworkPolicy) plus the security audit report; the runtime +# never opens any of them. compiled/prompt.txt and +# compiled/skills/skills.json are no longer generated (#147). +compiled/ +# Recursive — the Dockerfile shouldn't be baked into its own image. +Dockerfile +.dockerignore +# Build-only local binary stash; binaries land in /usr/local/bin via +# the bin stage already. +.local-bins/ ` ignorePath := filepath.Join(bc.Opts.OutputDir, ".dockerignore") if err := os.WriteFile(ignorePath, []byte(dockerignoreContent), 0644); err != nil { diff --git a/forge-cli/build/dockerfile_stage_issue147_test.go b/forge-cli/build/dockerfile_stage_issue147_test.go new file mode 100644 index 0000000..ff14c14 --- /dev/null +++ b/forge-cli/build/dockerfile_stage_issue147_test.go @@ -0,0 +1,158 @@ +package build + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/initializ/forge/forge-core/agentspec" + "github.com/initializ/forge/forge-core/pipeline" + "github.com/initializ/forge/forge-core/types" +) + +// TestDockerignore_ExcludesOperatorSideArtifacts pins the fix for +// issue #147 — the build's .dockerignore must keep operator-side +// artifacts (k8s manifests, build metadata, security audit report, +// the Dockerfile itself) out of the runtime container image. Pre-fix +// `COPY . .` blindly dumped all of them into /app/. +func TestDockerignore_ExcludesOperatorSideArtifacts(t *testing.T) { + _ = context.Background() + tmpDir := t.TempDir() + outDir := filepath.Join(tmpDir, "output") + if err := os.MkdirAll(outDir, 0755); err != nil { + t.Fatal(err) + } + + bc := pipeline.NewBuildContext(pipeline.PipelineOptions{WorkDir: tmpDir, OutputDir: outDir}) + bc.Config = &types.ForgeConfig{AgentID: "test", Version: "1.0.0"} + bc.Spec = &agentspec.AgentSpec{AgentID: "test"} + + s := &DockerfileStage{} + if err := s.writeDockerignore(bc); err != nil { + t.Fatalf("writeDockerignore: %v", err) + } + + data, err := os.ReadFile(filepath.Join(outDir, ".dockerignore")) + if err != nil { + t.Fatalf("reading .dockerignore: %v", err) + } + got := string(data) + + mustExclude := []string{ + // Secrets — preserved from the legacy list. + ".env", ".env.*", "*.enc", "secrets.enc", "*.key", "*.pem", + // Issue #147 additions. + "k8s/", + "build-manifest.json", + "compiled/", + "Dockerfile", + ".dockerignore", + ".local-bins/", + } + for _, p := range mustExclude { + if !strings.Contains(got, p+"\n") { + t.Errorf(".dockerignore is missing required exclusion %q. Full content:\n%s", p, got) + } + } +} + +// TestDockerignore_KeepsRuntimeFiles is the under-exclusion guard. +// The runtime opens forge.yaml, guardrails.json, -config.yaml, +// agent.json, policy-scaffold.json, skills/, checksums.json — none of +// those may appear as an exclusion pattern or the container would +// fail to start. +func TestDockerignore_KeepsRuntimeFiles(t *testing.T) { + tmpDir := t.TempDir() + outDir := filepath.Join(tmpDir, "output") + if err := os.MkdirAll(outDir, 0755); err != nil { + t.Fatal(err) + } + + bc := pipeline.NewBuildContext(pipeline.PipelineOptions{WorkDir: tmpDir, OutputDir: outDir}) + bc.Config = &types.ForgeConfig{AgentID: "test", Version: "1.0.0"} + bc.Spec = &agentspec.AgentSpec{AgentID: "test"} + + s := &DockerfileStage{} + if err := s.writeDockerignore(bc); err != nil { + t.Fatalf("writeDockerignore: %v", err) + } + + data, err := os.ReadFile(filepath.Join(outDir, ".dockerignore")) + if err != nil { + t.Fatalf("reading .dockerignore: %v", err) + } + got := string(data) + + mustNotExclude := []string{ + "forge.yaml", + "guardrails.json", + "agent.json", + "policy-scaffold.json", + "checksums.json", + "slack-config.yaml", + "telegram-config.yaml", + "msteams-config.yaml", + "skills/", + } + for _, p := range mustNotExclude { + for _, line := range strings.Split(got, "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "#") || trimmed == "" { + continue + } + if trimmed == p { + t.Errorf(".dockerignore must NOT exclude runtime-required file %q. Full content:\n%s", p, got) + } + } + } +} + +// TestDockerfile_NoLongerCopiesDeadCompiledArtifacts confirms the +// Dockerfile template no longer carries the redundant lines that +// copied compiled/prompt.txt and compiled/skills/skills.json into +// /app — those files are no longer generated (issue #147) so the +// COPYs would also fail on a real build. +func TestDockerfile_NoLongerCopiesDeadCompiledArtifacts(t *testing.T) { + tmpDir := t.TempDir() + outDir := filepath.Join(tmpDir, "output") + if err := os.MkdirAll(outDir, 0755); err != nil { + t.Fatal(err) + } + + bc := pipeline.NewBuildContext(pipeline.PipelineOptions{WorkDir: tmpDir, OutputDir: outDir}) + bc.Config = &types.ForgeConfig{ + AgentID: "test", + Version: "1.0.0", + Framework: "forge", + Entrypoint: "agent", + } + bc.Spec = &agentspec.AgentSpec{ + AgentID: "test", + Version: "1.0.0", + Runtime: &agentspec.RuntimeConfig{Image: "debian:bookworm-slim", Entrypoint: []string{"agent"}, Port: 8080}, + } + bc.SkillsCount = 2 + + s := &DockerfileStage{} + if err := s.generateTemplateDockerfile(bc); err != nil { + t.Fatalf("generateTemplateDockerfile: %v", err) + } + + data, err := os.ReadFile(filepath.Join(outDir, "Dockerfile")) + if err != nil { + t.Fatalf("reading Dockerfile: %v", err) + } + got := string(data) + + mustNotContain := []string{ + "COPY compiled/skills/", + "COPY compiled/prompt.txt", + } + for _, p := range mustNotContain { + if strings.Contains(got, p) { + t.Errorf("Dockerfile must not contain dead COPY %q (issue #147). Full content:\n%s", p, got) + } + } +} diff --git a/forge-cli/build/integration_test.go b/forge-cli/build/integration_test.go index 5d8725a..fa4f1d8 100644 --- a/forge-cli/build/integration_test.go +++ b/forge-cli/build/integration_test.go @@ -85,22 +85,21 @@ func TestIntegration_BuildWithSkillsAndEgress(t *testing.T) { t.Fatalf("pipeline.Run: %v", err) } - // Verify skills artifacts + // Issue #147: compiled/skills/skills.json and compiled/prompt.txt + // are no longer generated — the runtime never opened them and they + // bloated the container image. Assert they are NOT present so a + // future change reintroducing the dead writers is caught. The + // in-memory skill count is verified through bc.SkillsCount below. skillsPath := filepath.Join(outDir, "compiled", "skills", "skills.json") - if _, err := os.Stat(skillsPath); os.IsNotExist(err) { - t.Error("expected skills.json not found") - } else { - data, err := os.ReadFile(skillsPath) - if err != nil { - t.Fatalf("ReadFile: %v", err) - } - var skills map[string]any - if err := json.Unmarshal(data, &skills); err != nil { - t.Fatalf("unmarshal skills.json: %v", err) - } - if skills["count"].(float64) != 1 { - t.Errorf("skills count = %v, want 1", skills["count"]) - } + if _, err := os.Stat(skillsPath); err == nil { + t.Errorf("compiled/skills/skills.json should not be generated (issue #147)") + } + promptPath := filepath.Join(outDir, "compiled", "prompt.txt") + if _, err := os.Stat(promptPath); err == nil { + t.Errorf("compiled/prompt.txt should not be generated (issue #147)") + } + if bc.SkillsCount != 1 { + t.Errorf("bc.SkillsCount = %d, want 1", bc.SkillsCount) } // Verify egress allowlist diff --git a/forge-cli/build/skills_stage.go b/forge-cli/build/skills_stage.go index 9ba117f..3852bc1 100644 --- a/forge-cli/build/skills_stage.go +++ b/forge-cli/build/skills_stage.go @@ -8,7 +8,6 @@ import ( cliskills "github.com/initializ/forge/forge-cli/skills" "github.com/initializ/forge/forge-core/pipeline" - skillcompiler "github.com/initializ/forge/forge-skills/compiler" "github.com/initializ/forge/forge-skills/contract" "github.com/initializ/forge/forge-skills/requirements" ) @@ -61,23 +60,20 @@ func (s *SkillsStage) Execute(ctx context.Context, bc *pipeline.BuildContext) er bc.SkillRequirements = reqs } - compiled, err := skillcompiler.Compile(entries) - if err != nil { - return fmt.Errorf("compiling skills: %w", err) - } + // We deliberately do not write compiled/prompt.txt or + // compiled/skills/skills.json. The runtime re-globs skills/ + // SKILL.md on every startup (runner.discoverSkillFiles + + // buildSkillCatalog) and never opens either file. Generating + // them just bloated the build output and the container image + // for no consumer benefit. See issue #147 for the trace. + // External library consumers (forgecore.Compile) still get the + // in-memory CompiledSkills struct directly. - if err := cliskills.WriteArtifacts(bc.Opts.OutputDir, compiled); err != nil { - return fmt.Errorf("writing skills artifacts: %w", err) - } - - bc.SkillsCount = compiled.Count + bc.SkillsCount = len(entries) if bc.Spec != nil { bc.Spec.SkillsSpecVersion = "agentskills-v1" bc.Spec.ForgeSkillsExtVersion = "1.0" } - - bc.AddFile("compiled/skills/skills.json", filepath.Join(bc.Opts.OutputDir, "compiled", "skills", "skills.json")) - bc.AddFile("compiled/prompt.txt", filepath.Join(bc.Opts.OutputDir, "compiled", "prompt.txt")) return nil } diff --git a/forge-cli/build/skills_stage_test.go b/forge-cli/build/skills_stage_test.go index a40ab6f..f7042b7 100644 --- a/forge-cli/build/skills_stage_test.go +++ b/forge-cli/build/skills_stage_test.go @@ -64,12 +64,15 @@ Summarize text content. t.Errorf("SkillsSpecVersion = %q, want %q", bc.Spec.SkillsSpecVersion, "agentskills-v1") } - // Check artifacts exist - if _, err := os.Stat(filepath.Join(outDir, "compiled", "skills", "skills.json")); os.IsNotExist(err) { - t.Error("skills.json not created") + // Issue #147: the stage no longer writes compiled/skills/skills.json or + // compiled/prompt.txt — the runtime never opened them and they bloated + // the container image. Assert they are NOT present so a future change + // reintroducing the dead writers is caught. + if _, err := os.Stat(filepath.Join(outDir, "compiled", "skills", "skills.json")); err == nil { + t.Error("compiled/skills/skills.json should not be generated (issue #147)") } - if _, err := os.Stat(filepath.Join(outDir, "compiled", "prompt.txt")); os.IsNotExist(err) { - t.Error("prompt.txt not created") + if _, err := os.Stat(filepath.Join(outDir, "compiled", "prompt.txt")); err == nil { + t.Error("compiled/prompt.txt should not be generated (issue #147)") } } diff --git a/forge-cli/build/validate_stage.go b/forge-cli/build/validate_stage.go index 8dbe214..ff5dbaf 100644 --- a/forge-cli/build/validate_stage.go +++ b/forge-cli/build/validate_stage.go @@ -38,20 +38,6 @@ func (s *ValidateStage) Execute(ctx context.Context, bc *pipeline.BuildContext) } } - // Validate skills artifacts if skills were compiled - if bc.SkillsCount > 0 { - skillsArtifacts := []string{ - filepath.Join("compiled", "skills", "skills.json"), - filepath.Join("compiled", "prompt.txt"), - } - for _, f := range skillsArtifacts { - path := filepath.Join(bc.Opts.OutputDir, f) - if _, err := os.Stat(path); os.IsNotExist(err) { - return fmt.Errorf("expected skills artifact missing: %s", f) - } - } - } - // Validate egress artifact if egress was resolved if bc.EgressResolved != nil { egressPath := filepath.Join(bc.Opts.OutputDir, "compiled", "egress_allowlist.json") diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index 0ed6801..6055c74 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -242,7 +242,19 @@ func (r *Runner) Run(ctx context.Context) error { } // 0b. Verify build output integrity if checksums.json exists. + // Inside a Forge container, .forge-output/ is flattened into + // WorkDir (typically /app) — the .dockerignore drops the dir + // while keeping checksums.json at /app/checksums.json. On the + // operator side `forge run` is invoked next to forge.yaml, so + // the build output still lives under /.forge-output/. + // Try the operator-side layout first, then fall back to the + // flattened container layout (issue #147). outputDir := filepath.Join(r.cfg.WorkDir, ".forge-output") + if _, err := os.Stat(filepath.Join(outputDir, "checksums.json")); os.IsNotExist(err) { + if _, err := os.Stat(filepath.Join(r.cfg.WorkDir, "checksums.json")); err == nil { + outputDir = r.cfg.WorkDir + } + } if err := VerifyBuildOutput(outputDir); err != nil { r.logger.Warn("build output verification failed", map[string]any{"error": err.Error()}) } diff --git a/forge-cli/runtime/verify_issue147_test.go b/forge-cli/runtime/verify_issue147_test.go new file mode 100644 index 0000000..f64eae9 --- /dev/null +++ b/forge-cli/runtime/verify_issue147_test.go @@ -0,0 +1,102 @@ +package runtime + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "os" + "path/filepath" + "testing" +) + +// TestResolveChecksumsDir_OperatorLayoutPreferred and the companion +// TestResolveChecksumsDir_FlattenedContainerLayout pin the two paths +// the runner checks for checksums.json (issue #147). Pre-fix the +// runner only looked at /.forge-output/checksums.json, which +// never exists inside a Forge container — `COPY . .` from the +// .forge-output build context places the file at /app/checksums.json +// directly. Result: build-integrity verification was silently disabled +// in every Forge container. +// +// These tests don't exercise the full Runner.Start path; they exercise +// the helper logic that picks the directory passed to VerifyBuildOutput. + +// resolveChecksumsDir replicates the dir-resolution logic in +// Runner.Start so it can be tested without standing up a full runner. +// Keep in sync with runner.go § "0b. Verify build output integrity". +func resolveChecksumsDir(workDir string) string { + outputDir := filepath.Join(workDir, ".forge-output") + if _, err := os.Stat(filepath.Join(outputDir, "checksums.json")); os.IsNotExist(err) { + if _, err := os.Stat(filepath.Join(workDir, "checksums.json")); err == nil { + return workDir + } + } + return outputDir +} + +func TestResolveChecksumsDir_OperatorLayoutPreferred(t *testing.T) { + workDir := t.TempDir() + if err := os.MkdirAll(filepath.Join(workDir, ".forge-output"), 0755); err != nil { + t.Fatal(err) + } + // Operator-side layout: file under .forge-output/. + if err := os.WriteFile(filepath.Join(workDir, ".forge-output", "checksums.json"), []byte("{}"), 0644); err != nil { + t.Fatal(err) + } + + got := resolveChecksumsDir(workDir) + want := filepath.Join(workDir, ".forge-output") + if got != want { + t.Errorf("operator layout: got %q, want %q", got, want) + } +} + +func TestResolveChecksumsDir_FlattenedContainerLayout(t *testing.T) { + workDir := t.TempDir() + // Container layout: file at WorkDir root, no .forge-output/ dir. + if err := os.WriteFile(filepath.Join(workDir, "checksums.json"), []byte("{}"), 0644); err != nil { + t.Fatal(err) + } + + got := resolveChecksumsDir(workDir) + if got != workDir { + t.Errorf("container layout: got %q, want %q", got, workDir) + } +} + +func TestResolveChecksumsDir_NeitherPresent(t *testing.T) { + workDir := t.TempDir() + got := resolveChecksumsDir(workDir) + want := filepath.Join(workDir, ".forge-output") + if got != want { + t.Errorf("neither present: got %q, want %q (default to operator layout for VerifyBuildOutput's IsNotExist branch)", got, want) + } +} + +// TestVerifyBuildOutput_ContainerLayoutWorksEndToEnd is the +// integration check: a fully-flattened container directory (WorkDir +// containing checksums.json + the verified file alongside it) passes +// verification when callers feed WorkDir directly into +// VerifyBuildOutput, as the runner now does after the fallback. +func TestVerifyBuildOutput_ContainerLayoutWorksEndToEnd(t *testing.T) { + workDir := t.TempDir() + + content := []byte("forge: yaml: contents") + if err := os.WriteFile(filepath.Join(workDir, "forge.yaml"), content, 0644); err != nil { + t.Fatal(err) + } + h := sha256.Sum256(content) + cf := ChecksumsFile{ + Version: "1", + Checksums: map[string]string{"forge.yaml": hex.EncodeToString(h[:])}, + Timestamp: "2026-06-10T00:00:00Z", + } + data, _ := json.Marshal(cf) + if err := os.WriteFile(filepath.Join(workDir, "checksums.json"), data, 0644); err != nil { + t.Fatal(err) + } + + if err := VerifyBuildOutput(resolveChecksumsDir(workDir)); err != nil { + t.Fatalf("container-layout verification must succeed; got: %v", err) + } +} diff --git a/forge-cli/skills/writer.go b/forge-cli/skills/writer.go deleted file mode 100644 index bce38a7..0000000 --- a/forge-cli/skills/writer.go +++ /dev/null @@ -1,37 +0,0 @@ -package skills - -import ( - "encoding/json" - "fmt" - "os" - "path/filepath" - - "github.com/initializ/forge/forge-skills/contract" -) - -// WriteArtifacts creates compiled/skills/skills.json and compiled/prompt.txt in outputDir. -func WriteArtifacts(outputDir string, cs *contract.CompiledSkills) error { - skillsDir := filepath.Join(outputDir, "compiled", "skills") - if err := os.MkdirAll(skillsDir, 0755); err != nil { - return fmt.Errorf("creating skills directory: %w", err) - } - - // Write skills.json - data, err := json.MarshalIndent(cs, "", " ") - if err != nil { - return fmt.Errorf("marshalling skills: %w", err) - } - skillsPath := filepath.Join(skillsDir, "skills.json") - if err := os.WriteFile(skillsPath, data, 0644); err != nil { - return fmt.Errorf("writing skills.json: %w", err) - } - - // Write prompt.txt - compiledDir := filepath.Join(outputDir, "compiled") - promptPath := filepath.Join(compiledDir, "prompt.txt") - if err := os.WriteFile(promptPath, []byte(cs.Prompt), 0644); err != nil { - return fmt.Errorf("writing prompt.txt: %w", err) - } - - return nil -} diff --git a/forge-cli/templates/Dockerfile.tmpl b/forge-cli/templates/Dockerfile.tmpl index 0298288..1b127ee 100644 --- a/forge-cli/templates/Dockerfile.tmpl +++ b/forge-cli/templates/Dockerfile.tmpl @@ -73,11 +73,6 @@ RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certifi {{- end}} {{- end}} -{{- if .HasSkills}} -COPY compiled/skills/ /app/skills/ -COPY compiled/prompt.txt /app/compiled/prompt.txt -{{- end}} - {{- if .Runtime.HealthCheck}} HEALTHCHECK --interval=30s --timeout=5s --retries=3 \ CMD {{.Runtime.HealthCheck}} diff --git a/forge-skills/compiler/compiler.go b/forge-skills/compiler/compiler.go index 5d839ae..72f21ec 100644 --- a/forge-skills/compiler/compiler.go +++ b/forge-skills/compiler/compiler.go @@ -9,6 +9,16 @@ import ( ) // Compile converts parsed SkillEntry values into CompiledSkills. +// +// A SKILL.md with N `## Tool:` sections parses into N SkillEntry +// values that all share the same Body (the body is the trailing +// markdown after the frontmatter, not per-tool). Without dedup, the +// body lands in cs.Prompt N times — the bundled code-review-github +// skill (4 tools) produced 4× repeats; aibuilderdemo's prompt.txt was +// 1199 lines for what should be ~250 (issue #147). The seen-bodies +// set emits each body once. Per-skill JSON entries still carry the +// full Body because consumers of the in-memory CompiledSkill (e.g. +// forgecore SDK) may need it for non-prompt purposes. func Compile(entries []contract.SkillEntry) (*contract.CompiledSkills, error) { cs := &contract.CompiledSkills{ Skills: make([]contract.CompiledSkill, 0, len(entries)), @@ -18,6 +28,8 @@ func Compile(entries []contract.SkillEntry) (*contract.CompiledSkills, error) { var promptBuilder strings.Builder promptBuilder.WriteString("# Available Skills\n\n") + seenBodies := make(map[string]bool) + for _, e := range entries { skill := contract.CompiledSkill{ Name: e.Name, @@ -47,8 +59,9 @@ func Compile(entries []contract.SkillEntry) (*contract.CompiledSkills, error) { if e.OutputFormat != "" { fmt.Fprintf(&promptBuilder, "Output format: %s\n", e.OutputFormat) } - if e.Body != "" { + if e.Body != "" && !seenBodies[e.Body] { fmt.Fprintf(&promptBuilder, "\n%s\n", e.Body) + seenBodies[e.Body] = true } promptBuilder.WriteString("\n") } diff --git a/forge-skills/compiler/compiler_issue147_test.go b/forge-skills/compiler/compiler_issue147_test.go new file mode 100644 index 0000000..6aaeab4 --- /dev/null +++ b/forge-skills/compiler/compiler_issue147_test.go @@ -0,0 +1,94 @@ +package compiler + +import ( + "strings" + "testing" + + "github.com/initializ/forge/forge-skills/contract" +) + +// TestCompile_BodyDeduplication_MultipleToolsInOneSkillFile pins the +// fix for issue #147 — a SKILL.md with N `## Tool:` sections parses +// into N SkillEntry values that all share the same Body. Pre-fix the +// body landed in cs.Prompt N times. The bundled code-review-github +// skill has 4 tools and produced 4× repeats; aibuilderdemo's +// prompt.txt was 1199 lines for what should be ~250. +func TestCompile_BodyDeduplication_MultipleToolsInOneSkillFile(t *testing.T) { + sharedBody := "# Code Review Skill\n\nDetailed instructions and examples...\n" + entries := []contract.SkillEntry{ + {Name: "code_review_diff", Description: "Diff review", Body: sharedBody}, + {Name: "code_review_file", Description: "File review", Body: sharedBody}, + } + + cs, err := Compile(entries) + if err != nil { + t.Fatalf("Compile: %v", err) + } + + // The body marker should appear exactly once in the prompt — not + // once per tool entry. + occurrences := strings.Count(cs.Prompt, "Detailed instructions and examples") + if occurrences != 1 { + t.Errorf("shared body should be emitted exactly once, got %d occurrences. Prompt:\n%s", + occurrences, cs.Prompt) + } + + // Both tool names must still appear (only the body dedupes). + if !strings.Contains(cs.Prompt, "code_review_diff") || !strings.Contains(cs.Prompt, "code_review_file") { + t.Errorf("both tool names must appear in prompt catalog; got:\n%s", cs.Prompt) + } + + // Per-skill JSON entries still carry the full Body — SDK consumers + // of CompiledSkills may need it for non-prompt purposes. + for i, sk := range cs.Skills { + if sk.Body != sharedBody { + t.Errorf("Skills[%d].Body = %q, want full shared body", i, sk.Body) + } + } +} + +// TestCompile_BodyDeduplication_DistinctBodiesPreserved is the +// over-collapse guard: bodies that differ must both appear in the +// prompt. Only EXACT identical bodies dedup. +func TestCompile_BodyDeduplication_DistinctBodiesPreserved(t *testing.T) { + entries := []contract.SkillEntry{ + {Name: "tool_a", Description: "A", Body: "Body for skill A"}, + {Name: "tool_b", Description: "B", Body: "Body for skill B"}, + } + cs, err := Compile(entries) + if err != nil { + t.Fatalf("Compile: %v", err) + } + if !strings.Contains(cs.Prompt, "Body for skill A") { + t.Errorf("body A must appear; got:\n%s", cs.Prompt) + } + if !strings.Contains(cs.Prompt, "Body for skill B") { + t.Errorf("body B must appear; got:\n%s", cs.Prompt) + } +} + +// TestCompile_BodyDeduplication_RealisticCodeReviewShape mirrors the +// bundled code-review-github skill (4 tools sharing one ~50-line +// body). Pre-fix the body landed in prompt.txt 4 times; this test +// guards the runtime against regressing to that shape. +func TestCompile_BodyDeduplication_RealisticCodeReviewShape(t *testing.T) { + body := strings.Repeat("Detailed skill instructions line.\n", 50) + entries := []contract.SkillEntry{ + {Name: "review_github_list_prs", Description: "List PRs", Body: body}, + {Name: "review_github_post_comments", Description: "Post comments", Body: body}, + {Name: "review_github_apply_labels", Description: "Apply labels", Body: body}, + {Name: "review_github_auto_merge", Description: "Auto-merge", Body: body}, + } + cs, err := Compile(entries) + if err != nil { + t.Fatalf("Compile: %v", err) + } + + // Body has 50 identical lines. Pre-fix it would appear 4× → 200 + // lines of body content. Post-fix it appears 1× → 50 lines. + lineCount := strings.Count(cs.Prompt, "Detailed skill instructions line.") + if lineCount != 50 { + t.Errorf("expected body to appear once (50 lines); got %d lines. "+ + "That implies %dx duplication.", lineCount, lineCount/50) + } +}