From bdee2865403731b9dc72d5c5f78c47de1438756e Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 10 Jun 2026 19:54:22 -0400 Subject: [PATCH 1/3] fix(build): kill dead compiled artifacts, tighten Dockerfile context, fix checksums path (closes #147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five interlocking bugs in the `forge build` output that compound to a bloated runtime image and a silently-disabled integrity check: 1. **Prompt-builder body duplication.** A SKILL.md with N `## Tool:` sections parses into N SkillEntry values that all share the same Body. The compiler's prompt builder appended the body once per entry, so the bundled code-review-github skill (4 tools) shipped 4× repeats and aibuilderdemo's compiled/prompt.txt was 1199 lines for what should be ~250. Dedup via a seen-bodies set; per-skill JSON entries still carry the full Body for SDK consumers. 2. **compiled/prompt.txt and compiled/skills/skills.json are dead.** The runtime never opens either file — runner.discoverSkillFiles() re-globs skills/SKILL.md on every startup. Stopped generating them; deleted forge-cli/skills/writer.go; pruned the matching COPY lines from the Dockerfile template and the validate stage's required-files check. External library consumers (forgecore.Compile) still receive the in-memory CompiledSkills struct directly. 3. **Dockerfile `COPY . .` leaked operator-side artifacts into /app.** Expanded .dockerignore to exclude k8s/, build-manifest.json, the entire compiled/ directory, the Dockerfile itself, the .dockerignore itself, and .local-bins/. The runtime image now contains only forge.yaml, guardrails.json, -config.yaml, agent.json, policy-scaffold.json, skills/, and checksums.json. 4. **checksums.json integrity check silently skipped in container.** The runner looked at /.forge-output/checksums.json, which never exists inside a Forge container — the file lands at /app/checksums.json directly. Added a fallback that picks the flattened layout when the operator-side layout is absent. 5. **Removed redundant COPY lines from Dockerfile.tmpl** that would have failed after fix #2 anyway (the targets no longer exist). End-to-end verified on the user's aibuilderdemo agent: - Pre-fix: 21 files in .forge-output, compiled/prompt.txt 1199 lines, image contained k8s/ + build-manifest.json + security-audit.json + recursive Dockerfile + dead compiled/ artifacts. - Post-fix: 19 files (compiled/prompt.txt + compiled/skills/skills.json removed), image contains only the 8 files the runtime opens, operator artifacts intact on disk for forge package / kubectl apply. Tests pin all four invariants: - TestCompile_BodyDeduplication_* (3 tests) - TestSkillsStage_WithSkills updated to assert artifacts NOT created - TestDockerignore_ExcludesOperatorSideArtifacts + TestDockerignore_KeepsRuntimeFiles + TestDockerfile_NoLongerCopiesDeadCompiledArtifacts - TestResolveChecksumsDir_* (3 tests) + TestVerifyBuildOutput_ContainerLayoutWorksEndToEnd --- forge-cli/build/dockerfile_stage.go | 29 +++- .../build/dockerfile_stage_issue147_test.go | 158 ++++++++++++++++++ forge-cli/build/skills_stage.go | 22 +-- forge-cli/build/skills_stage_test.go | 13 +- forge-cli/build/validate_stage.go | 14 -- forge-cli/runtime/runner.go | 12 ++ forge-cli/runtime/verify_issue147_test.go | 102 +++++++++++ forge-cli/skills/writer.go | 37 ---- forge-cli/templates/Dockerfile.tmpl | 5 - forge-skills/compiler/compiler.go | 15 +- .../compiler/compiler_issue147_test.go | 94 +++++++++++ 11 files changed, 425 insertions(+), 76 deletions(-) create mode 100644 forge-cli/build/dockerfile_stage_issue147_test.go create mode 100644 forge-cli/runtime/verify_issue147_test.go delete mode 100644 forge-cli/skills/writer.go create mode 100644 forge-skills/compiler/compiler_issue147_test.go 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/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) + } +} From c0dba7e0bb14f2b40871fad370faa0e2f6c760f1 Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 10 Jun 2026 20:05:19 -0400 Subject: [PATCH 2/3] fix(build): apt bins to app stage, per-binary COPYs, no more lost runtime deps (closes #149) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apt-installed binaries declared by skills were silently lost in the final image. The pre-fix flow: every skill bin (curl, git, jq, gh) went through a single shared `bins` stage. apt placed curl/git/jq at /usr/bin/ (Debian's default), but the application stage only did `COPY --from=bins /usr/local/bin/ /usr/local/bin/` — picking up just the manually-placed `gh` and dropping the apt-installed bins entirely. Any skill script that called curl/git/jq would log "command not found" at runtime; only `gh` worked because its install path explicitly moved it to /usr/local/bin/gh. The blunt wholesale-directory COPY also hid the bug — there was no per-binary contract to assert against. New code, new artifact, easy to miss in review. This change separates the bins-stage and app-stage concerns: - apt/apk binaries → installed in the application stage directly via the existing `RUN apt-get install` slot. The package manager's dependency resolution pulls /usr/lib/, /etc/, etc. along automatically, so per-bin `COPY --from=bins` plumbing (which can't preserve lib deps) is avoided. - direct-URL + custom-RUN binaries → still flow through the shared `bins` stage (they need curl + a writable filesystem to download and unpack). The bins stage's apt install is now strictly scoped to build-time helpers (curl + ca-certificates) that never reach the app image. - image-copy binaries (e.g. playwright) → copied straight from their `bin-` companion stage to the app stage, skipping the bins intermediate hop. - local-file binaries → emitted as `COPY .local-bins/` in the app stage (no `--from`, no bins-stage routing). `packaging.GenerateDockerfile` now returns a structured `DockerfileFragments` with separate slots for `PreAppStages`, `RuntimeAptPackages`, `RuntimeApkPackages`, `BinCopies`, and `PathExtensions`. The template consumes each slot in the right place; the wholesale `COPY --from=bins /usr/local/bin/ /usr/local/bin/` is gone, replaced by an explicit per-binary loop. End-to-end verified on the bundled aibuilderdemo agent (curl/git/jq via apt, gh via custom-RUN). The pre-fix Dockerfile had curl/git/jq trapped in the bins stage and dropped on COPY. Post-fix: FROM debian:bookworm-slim AS bins RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates && rm -rf /var/lib/apt/lists/* RUN curl -fsSL https://github.com/cli/cli/releases/.../gh_2.60.0_linux_amd64.tar.gz | tar xz -C /tmp RUN mv /tmp/.../gh /usr/local/bin/gh RUN chmod 0755 /usr/local/bin/gh # --- Application stage --- FROM debian:bookworm-slim ... WORKDIR /app COPY --from=bins /usr/local/bin/gh /usr/local/bin/gh COPY . . RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates curl git jq && rm -rf /var/lib/apt/lists/* ... ENTRYPOINT ["forge","run","--host","0.0.0.0","--with","slack"] Tests pin the new contract per install method (apt/apk in app stage, direct-URL/custom-RUN via per-binary COPY from bins, image-copy via per-binary COPY from companion stage, local-file from build context), plus a regression test for the bundled code-review skill shape (curl+git+jq+gh) confirming all four reach the final image. --- forge-cli/build/dockerfile_stage.go | 26 +- forge-cli/templates/Dockerfile.tmpl | 12 +- forge-core/compiler/template_data.go | 8 + forge-core/packaging/dockerfile_generator.go | 278 +++++++++++------- .../packaging/dockerfile_generator_test.go | 216 +++++++++++--- 5 files changed, 376 insertions(+), 164 deletions(-) diff --git a/forge-cli/build/dockerfile_stage.go b/forge-cli/build/dockerfile_stage.go index 1066069..a9b77ee 100644 --- a/forge-cli/build/dockerfile_stage.go +++ b/forge-cli/build/dockerfile_stage.go @@ -57,7 +57,7 @@ func (s *DockerfileStage) Execute(ctx context.Context, bc *pipeline.BuildContext func (s *DockerfileStage) generateSmartDockerfile(bc *pipeline.BuildContext, manifest *packaging.BinManifest) error { cfg := bc.Config.Package - binFragment, warnings, err := packaging.GenerateDockerfile(manifest, cfg, bc.PreferAlpine, bc.PreferSlim) + frags, warnings, err := packaging.GenerateDockerfile(manifest, cfg, bc.PreferAlpine, bc.PreferSlim) if err != nil { return fmt.Errorf("generating bin install Dockerfile: %w", err) } @@ -70,8 +70,12 @@ func (s *DockerfileStage) generateSmartDockerfile(bc *pipeline.BuildContext, man fmt.Fprintf(os.Stderr, " [bins] resolved %d binaries\n", len(manifest.Requirements)) - // Now generate the main Dockerfile incorporating the bin fragment - // The bin fragment is a separate stage; we prepend it to the existing template output + // Render the application stage from the template, feeding the + // per-binary plumbing (BinCopies, RuntimeAptPackages, etc.) so the + // template can place each in the right slot. The blunt + // COPY --from=bins /usr/local/bin/ /usr/local/bin/ that used to + // hide apt-installed-binary losses is gone — every binary that + // reaches the app stage has an explicit COPY line (issue #149). tmplData, err := templates.FS.ReadFile("Dockerfile.tmpl") if err != nil { return fmt.Errorf("reading Dockerfile template: %w", err) @@ -83,18 +87,24 @@ func (s *DockerfileStage) generateSmartDockerfile(bc *pipeline.BuildContext, man } data := compiler.BuildTemplateDataFromContext(bc.Spec, bc) - data.HasBinStage = true + data.HasBinStage = frags.HasPreAppStages() + data.BinCopies = frags.BinCopies + data.RuntimeAptPackages = frags.RuntimeAptPackages + data.RuntimeApkPackages = frags.RuntimeApkPackages + data.PathExtensions = frags.PathExtensions var buf bytes.Buffer if err := tmpl.Execute(&buf, data); err != nil { return fmt.Errorf("rendering Dockerfile: %w", err) } - // Combine: bin install stages + main Dockerfile + // Combine: pre-app stages + main Dockerfile. var combined bytes.Buffer - combined.WriteString("# --- Binary installation stages (auto-generated) ---\n") - combined.WriteString(binFragment) - combined.WriteString("\n# --- Application stage ---\n") + if frags.HasPreAppStages() { + combined.WriteString("# --- Binary installation stages (auto-generated) ---\n") + combined.WriteString(frags.PreAppStages) + combined.WriteString("\n# --- Application stage ---\n") + } combined.Write(buf.Bytes()) outPath := filepath.Join(bc.Opts.OutputDir, "Dockerfile") diff --git a/forge-cli/templates/Dockerfile.tmpl b/forge-cli/templates/Dockerfile.tmpl index 1b127ee..c522183 100644 --- a/forge-cli/templates/Dockerfile.tmpl +++ b/forge-cli/templates/Dockerfile.tmpl @@ -45,8 +45,8 @@ ENV {{$key}}="{{$val}}" ARG FORGE_DEV=false WORKDIR /app -{{- if .HasBinStage}} -COPY --from=bins /usr/local/bin/ /usr/local/bin/ +{{- range .BinCopies}} +{{.}} {{- end}} {{- if .Runtime.DepsFile}} @@ -54,7 +54,10 @@ COPY --from=deps /app/ . {{- end}} COPY . . -RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates && rm -rf /var/lib/apt/lists/* +RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates{{range .RuntimeAptPackages}} {{.}}{{end}} && rm -rf /var/lib/apt/lists/* +{{- if .RuntimeApkPackages}} +RUN apk add --no-cache{{range .RuntimeApkPackages}} {{.}}{{end}} +{{- end}} {{- if .ForgeFramework}} {{- if eq .ForgeVersion "latest"}} RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates && \ @@ -86,5 +89,8 @@ EXPOSE {{.Runtime.Port}} RUN useradd -r -s /bin/false agent && chown -R agent:agent /app USER agent {{- end}} +{{- range .PathExtensions}} +ENV PATH="{{.}}:$PATH" +{{- end}} ENTRYPOINT {{.Runtime.Entrypoint}} diff --git a/forge-core/compiler/template_data.go b/forge-core/compiler/template_data.go index ba0dfee..03c5a6d 100644 --- a/forge-core/compiler/template_data.go +++ b/forge-core/compiler/template_data.go @@ -35,6 +35,14 @@ type TemplateSpecData struct { // Multi-stage build HasBinStage bool // true when a bins stage exists with installed binaries + + // Per-binary plumbing emitted in the application stage. See + // forge-core/packaging.DockerfileFragments for the full design + // (issue #149). + BinCopies []string // "COPY --from= /path /path" lines, one per binary. + RuntimeAptPackages []string // runtime apt packages installed in the application stage (debian). + RuntimeApkPackages []string // runtime apk packages installed in the application stage (alpine). + PathExtensions []string // PATH directories for non-standard binary locations. } // TemplateRuntimeData holds runtime-specific template data. diff --git a/forge-core/packaging/dockerfile_generator.go b/forge-core/packaging/dockerfile_generator.go index 9de8c13..a8afc87 100644 --- a/forge-core/packaging/dockerfile_generator.go +++ b/forge-core/packaging/dockerfile_generator.go @@ -7,108 +7,150 @@ import ( "github.com/initializ/forge/forge-core/types" ) -// GenerateDockerfile produces a Dockerfile with actual install commands for all resolved binaries. -// Returns (dockerfile content, warnings, error). -func GenerateDockerfile(manifest *BinManifest, cfg types.PackageConfig, alpine, slim bool) (string, []string, error) { +// DockerfileFragments is the structured output of GenerateDockerfile. +// It separates the multi-stage pre-application chunks (companion +// image-copy stages + a shared bins stage for direct-URL / custom-RUN +// downloads) from the per-binary plumbing the application stage needs. +// +// Pre-issue #149 the generator returned a single string for "everything +// before the application stage" and the template emitted one blunt +// `COPY --from=bins /usr/local/bin/ /usr/local/bin/`. apt-installed +// binaries land at /usr/bin/ on Debian (not /usr/local/bin/) and have +// transitive lib/etc deps, so they could never survive the per-stage +// COPY — and the wholesale directory copy hid the issue from review. +// +// Post-fix: +// - Apt/apk binaries are installed in the application stage directly, +// so the package manager's dependency resolution pulls in the right +// shared libs and config. +// - Direct-URL / custom-RUN / local-file binaries still flow through a +// shared `bins` stage (they need curl + a writable filesystem) and +// are forwarded via per-binary explicit COPYs. +// - Image-copy binaries skip the bins stage entirely and copy straight +// from their dedicated `bin-` companion stage to the app stage. +type DockerfileFragments struct { + // PreAppStages is the Dockerfile chunk preceding the application + // stage. May contain companion `FROM AS bin-` + // stages for image-copy binaries plus a shared `bins` stage for + // direct-URL / custom-RUN / local-file installs. Empty when no + // binary needs a pre-app stage (e.g. an agent declaring only + // apt-installable bins). + PreAppStages string + + // RuntimeAptPackages are runtime apt packages the application + // stage must install. Debian-only; nil on Alpine. + RuntimeAptPackages []string + + // RuntimeApkPackages are runtime apk packages the application + // stage must install. Alpine-only; nil on Debian. + RuntimeApkPackages []string + + // BinCopies are formatted "COPY --from= " + // lines emitted by the application stage. One per binary — + // intent-explicit, no wholesale-directory copies. Empty when no + // pre-app stage produced any binary. + BinCopies []string + + // PathExtensions are PATH directories the application stage + // exports for binaries installed at non-standard locations. + PathExtensions []string +} + +// HasPreAppStages reports whether any pre-application multi-stage +// content exists. Used by the template to decide whether to emit the +// "# --- Binary installation stages ---" header. +func (f DockerfileFragments) HasPreAppStages() bool { + return strings.TrimSpace(f.PreAppStages) != "" +} + +// GenerateDockerfile classifies the agent's binary requirements and +// returns the per-stage Dockerfile fragments. Callers compose the +// final Dockerfile by emitting PreAppStages, then the application +// stage with the BinCopies / RuntimeAptPackages / RuntimeApkPackages +// hooks honored. +// +// Returns (fragments, warnings, error). A nil/empty manifest returns +// zero-value fragments and no error. +func GenerateDockerfile(manifest *BinManifest, cfg types.PackageConfig, alpine, slim bool) (DockerfileFragments, []string, error) { if manifest == nil || len(manifest.Requirements) == 0 { - return "", nil, nil + return DockerfileFragments{}, nil, nil } classifier, err := NewBinClassifier(cfg, slim, alpine) if err != nil { - return "", nil, err + return DockerfileFragments{}, nil, err } resolutions, warnings, err := classifier.Classify(manifest) if err != nil { - return "", nil, fmt.Errorf("classifying binaries: %w", err) + return DockerfileFragments{}, nil, fmt.Errorf("classifying binaries: %w", err) } baseImg := SelectBaseImage(resolutions, cfg.BaseImage, alpine) - - // Check if alpine was requested but blocked if alpine && !baseImg.IsAlpine { warnings = append(warnings, "Alpine base image requested but a binary requires Ubuntu; using debian:bookworm-slim") } - var b strings.Builder + out := DockerfileFragments{} - // Companion stages for heavy (image-copy) binaries - for _, r := range resolutions { - if r.Method == MethodImageCopy && r.Image != "" { - fmt.Fprintf(&b, "FROM %s AS bin-%s\n\n", r.Image, r.Name) - } - } - - // Base stage - fmt.Fprintf(&b, "FROM %s AS bins\n", baseImg.Image) - - // Batch apt/apk packages - var aptPkgs, apkPkgs []string - hasDirectURL := false + // Partition resolutions by install method. + var ( + runtimeAptPkgs []string + runtimeApkPkgs []string + directURLs []BinResolution + customRuns []BinResolution + localFiles []BinResolution + imageCopies []BinResolution + ) for _, r := range resolutions { switch r.Method { case MethodApt: - aptPkgs = append(aptPkgs, r.Package) + runtimeAptPkgs = append(runtimeAptPkgs, r.Package) case MethodApk: - apkPkgs = append(apkPkgs, r.Package) + runtimeApkPkgs = append(runtimeApkPkgs, r.Package) case MethodDirectURL: - hasDirectURL = true + directURLs = append(directURLs, r) + case MethodCustomRun: + customRuns = append(customRuns, r) + case MethodLocalFile: + localFiles = append(localFiles, r) + case MethodImageCopy: + imageCopies = append(imageCopies, r) } } - - // Ensure curl and ca-certificates are available for direct URL downloads - if hasDirectURL && !alpine { - hasCurl := false - for _, p := range aptPkgs { - if p == "curl" { - hasCurl = true - break - } - } - if !hasCurl { - aptPkgs = append([]string{"curl", "ca-certificates"}, aptPkgs...) + out.RuntimeAptPackages = runtimeAptPkgs + out.RuntimeApkPackages = runtimeApkPkgs + + // Build the pre-application stages. Two chunks: + // (1) Companion `FROM AS bin-` stages for image-copy bins. + // (2) A shared `bins` stage that hosts curl-based direct-URL + // downloads + custom RUNs. Local-file copies do NOT need a + // bins stage — they're COPY-from-context, so they run in the + // application stage directly. + var pre strings.Builder + for _, r := range imageCopies { + if r.Image != "" { + fmt.Fprintf(&pre, "FROM %s AS bin-%s\n\n", r.Image, r.Name) } } - if hasDirectURL && alpine { - hasCurl := false - for _, p := range apkPkgs { - if p == "curl" { - hasCurl = true - break - } - } - if !hasCurl { - apkPkgs = append([]string{"curl", "ca-certificates"}, apkPkgs...) - } - } - - if len(aptPkgs) > 0 { - fmt.Fprintf(&b, "RUN apt-get update && apt-get install -y --no-install-recommends \\\n") - for i, pkg := range aptPkgs { - if i < len(aptPkgs)-1 { - fmt.Fprintf(&b, " %s \\\n", pkg) - } else { - fmt.Fprintf(&b, " %s \\\n", pkg) - } - } - fmt.Fprintf(&b, " && rm -rf /var/lib/apt/lists/*\n") - } - if len(apkPkgs) > 0 { - fmt.Fprintf(&b, "RUN apk add --no-cache \\\n") - for i, pkg := range apkPkgs { - if i < len(apkPkgs)-1 { - fmt.Fprintf(&b, " %s \\\n", pkg) - } else { - fmt.Fprintf(&b, " %s\n", pkg) - } + needsSharedBinsStage := len(directURLs) > 0 || len(customRuns) > 0 + if needsSharedBinsStage { + fmt.Fprintf(&pre, "FROM %s AS bins\n", baseImg.Image) + + // curl + ca-certificates are build-time helpers. Direct-URL + // downloads use curl directly; custom-RUN scripts (e.g. the + // `gh` install which pipes `curl | tar xz`) need them too. + // They are scoped to the bins stage — the application stage + // does not inherit them, so this install does not pollute the + // runtime image. + if alpine { + pre.WriteString("RUN apk add --no-cache curl ca-certificates\n") + } else { + pre.WriteString("RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates && rm -rf /var/lib/apt/lists/*\n") } - } - // Direct URL downloads - for _, r := range resolutions { - if r.Method == MethodDirectURL { + for _, r := range directURLs { dest := r.Dest if dest == "" { dest = "/usr/local/bin/" + r.Name @@ -117,59 +159,75 @@ func GenerateDockerfile(manifest *BinManifest, cfg types.PackageConfig, alpine, if chmod == "" { chmod = "0755" } - fmt.Fprintf(&b, "RUN curl -fsSL %q -o %s && chmod %s %s\n", r.URL, dest, chmod, dest) + fmt.Fprintf(&pre, "RUN curl -fsSL %q -o %s && chmod %s %s\n", r.URL, dest, chmod, dest) } - } - - // Custom RUN lines - for _, r := range resolutions { - if r.Method == MethodCustomRun { + for _, r := range customRuns { for _, line := range r.RunLines { - fmt.Fprintf(&b, "RUN %s\n", line) + fmt.Fprintf(&pre, "RUN %s\n", line) } } } - - // Image-copy COPY instructions - for _, r := range resolutions { - if r.Method == MethodImageCopy { - dest := r.Dest - if dest == "" { - dest = "/usr/local/bin/" + r.Name - } - fmt.Fprintf(&b, "COPY --from=bin-%s %s %s\n", r.Name, dest, dest) + out.PreAppStages = pre.String() + + // Build the per-binary COPY lines for the application stage. + // - Direct-URL / custom-RUN binaries come from the shared `bins` stage. + // - Image-copy binaries come straight from their `bin-` companion + // stage, skipping the bins-stage intermediate hop. + // - Local-file binaries are COPY-from-build-context, emitted in the app + // stage (no --from). + var copies []string + for _, r := range directURLs { + dest := r.Dest + if dest == "" { + dest = "/usr/local/bin/" + r.Name } + copies = append(copies, fmt.Sprintf("COPY --from=bins %s %s", dest, dest)) } - - // Local file COPY instructions - for _, r := range resolutions { - if r.Method == MethodLocalFile { - dest := r.Dest - if dest == "" { - dest = "/usr/local/bin/" + r.Name - } - chmod := r.Chmod - if chmod == "" { - chmod = "0755" - } - fmt.Fprintf(&b, "COPY .local-bins/%s %s\n", r.Name, dest) - fmt.Fprintf(&b, "RUN chmod %s %s\n", chmod, dest) + for _, r := range customRuns { + dest := r.Dest + if dest == "" { + dest = "/usr/local/bin/" + r.Name + } + copies = append(copies, fmt.Sprintf("COPY --from=bins %s %s", dest, dest)) + } + for _, r := range imageCopies { + dest := r.Dest + if dest == "" { + dest = "/usr/local/bin/" + r.Name + } + copies = append(copies, fmt.Sprintf("COPY --from=bin-%s %s %s", r.Name, dest, dest)) + } + for _, r := range localFiles { + dest := r.Dest + if dest == "" { + dest = "/usr/local/bin/" + r.Name } + chmod := r.Chmod + if chmod == "" { + chmod = "0755" + } + // Local files use COPY-from-build-context; emit the COPY + + // chmod pair so the binary lands executable. + copies = append(copies, fmt.Sprintf("COPY .local-bins/%s %s", r.Name, dest)) + copies = append(copies, fmt.Sprintf("RUN chmod %s %s", chmod, dest)) } + out.BinCopies = copies - // PATH extensions + // PATH extensions for binaries installed at non-standard locations. var pathExts []string for _, r := range resolutions { if r.Dest != "" && r.Dest != "/usr/local/bin/"+r.Name { - dir := r.Dest[:strings.LastIndex(r.Dest, "/")] - if dir != "" && dir != "/usr/local/bin" && dir != "/usr/bin" { + idx := strings.LastIndex(r.Dest, "/") + if idx <= 0 { + continue + } + dir := r.Dest[:idx] + if dir != "/usr/local/bin" && dir != "/usr/bin" { pathExts = append(pathExts, dir) } } } - if len(pathExts) > 0 { - fmt.Fprintf(&b, "ENV PATH=\"%s:$PATH\"\n", strings.Join(pathExts, ":")) - } + out.PathExtensions = pathExts - return b.String(), warnings, nil + return out, warnings, nil } diff --git a/forge-core/packaging/dockerfile_generator_test.go b/forge-core/packaging/dockerfile_generator_test.go index 4ba34e0..07f5ffc 100644 --- a/forge-core/packaging/dockerfile_generator_test.go +++ b/forge-core/packaging/dockerfile_generator_test.go @@ -9,19 +9,28 @@ import ( ) func TestGenerateDockerfile_NoBins(t *testing.T) { - content, warnings, err := GenerateDockerfile(nil, types.PackageConfig{}, false, false) + frags, warnings, err := GenerateDockerfile(nil, types.PackageConfig{}, false, false) if err != nil { t.Fatalf("unexpected error: %v", err) } - if content != "" { - t.Errorf("expected empty content, got %q", content) + if frags.PreAppStages != "" { + t.Errorf("expected empty PreAppStages, got %q", frags.PreAppStages) + } + if len(frags.BinCopies) != 0 { + t.Errorf("expected no bin copies, got %v", frags.BinCopies) } if len(warnings) != 0 { t.Errorf("expected no warnings, got %v", warnings) } } -func TestGenerateDockerfile_AptBatch(t *testing.T) { +// TestGenerateDockerfile_AptBatch_RunsInAppStage pins the fix for +// issue #149: apt-installed binaries must NOT be routed through the +// bins stage (where they'd land in /usr/bin/ and never reach the +// application image), they must be returned as RuntimeAptPackages +// for the app stage to install directly. Per-binary lib/etc deps +// then come along via apt's dependency resolution. +func TestGenerateDockerfile_AptBatch_RunsInAppStage(t *testing.T) { manifest := &BinManifest{ Requirements: []contract.BinRequirement{ {Name: "jq"}, @@ -29,101 +38,157 @@ func TestGenerateDockerfile_AptBatch(t *testing.T) { }, } - content, _, err := GenerateDockerfile(manifest, types.PackageConfig{}, false, false) + frags, _, err := GenerateDockerfile(manifest, types.PackageConfig{}, false, false) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(content, "FROM debian:bookworm-slim") { - t.Error("expected debian:bookworm-slim base image") + // PreAppStages should be empty — no bins stage is needed for + // apt-only requirements (the agent's apt deps are installed in the + // application stage where they can reach the runtime). + if frags.PreAppStages != "" { + t.Errorf("apt-only requirements should not emit a bins stage; got PreAppStages:\n%s", frags.PreAppStages) } - if !strings.Contains(content, "apt-get install") { - t.Error("expected apt-get install") + + // Runtime apt packages must include both declared bins. + wantPkgs := map[string]bool{"jq": false, "curl": false} + for _, p := range frags.RuntimeAptPackages { + if _, ok := wantPkgs[p]; ok { + wantPkgs[p] = true + } } - if !strings.Contains(content, "jq") { - t.Error("expected jq package") + for pkg, found := range wantPkgs { + if !found { + t.Errorf("RuntimeAptPackages missing %q; got %v", pkg, frags.RuntimeAptPackages) + } } - if !strings.Contains(content, "curl") { - t.Error("expected curl package") + + // No BinCopies for apt installs — apt does the placement at /usr/bin. + for _, c := range frags.BinCopies { + if strings.Contains(c, "jq") || strings.Contains(c, "curl") { + t.Errorf("apt-installed bins should not appear in BinCopies; got %q", c) + } } } -func TestGenerateDockerfile_Alpine(t *testing.T) { +func TestGenerateDockerfile_Alpine_ApkRunsInAppStage(t *testing.T) { manifest := &BinManifest{ Requirements: []contract.BinRequirement{ {Name: "jq"}, }, } - content, _, err := GenerateDockerfile(manifest, types.PackageConfig{}, true, false) + frags, _, err := GenerateDockerfile(manifest, types.PackageConfig{}, true, false) if err != nil { t.Fatalf("unexpected error: %v", err) } - - if !strings.Contains(content, "FROM alpine:3.20") { - t.Error("expected alpine:3.20 base image") + if frags.PreAppStages != "" { + t.Errorf("apk-only requirements should not emit a bins stage; got:\n%s", frags.PreAppStages) } - if !strings.Contains(content, "apk add") { - t.Error("expected apk add") + if len(frags.RuntimeApkPackages) == 0 || frags.RuntimeApkPackages[0] != "jq" { + t.Errorf("expected RuntimeApkPackages=[jq], got %v", frags.RuntimeApkPackages) } } -func TestGenerateDockerfile_DirectURL(t *testing.T) { +// TestGenerateDockerfile_DirectURL_RoutesThroughBinsStage confirms +// direct-URL binaries (e.g. kubectl from GitHub releases) flow through +// the shared bins stage and get forwarded with an explicit per-binary +// COPY — no more wholesale-directory copies. +func TestGenerateDockerfile_DirectURL_RoutesThroughBinsStage(t *testing.T) { manifest := &BinManifest{ Requirements: []contract.BinRequirement{ {Name: "kubectl"}, }, } - content, _, err := GenerateDockerfile(manifest, types.PackageConfig{}, false, false) + frags, _, err := GenerateDockerfile(manifest, types.PackageConfig{}, false, false) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(content, "curl -fsSL") { - t.Error("expected curl download command for kubectl") + if !strings.Contains(frags.PreAppStages, "FROM debian:bookworm-slim AS bins") { + t.Errorf("direct-URL bin should produce a bins stage; got PreAppStages:\n%s", frags.PreAppStages) + } + if !strings.Contains(frags.PreAppStages, "curl -fsSL") { + t.Errorf("expected curl download in bins stage; got:\n%s", frags.PreAppStages) + } + // Build-time curl/ca-certificates install in bins stage (scoped to + // the stage; never propagates to the app image). + if !strings.Contains(frags.PreAppStages, "apt-get install -y --no-install-recommends curl ca-certificates") { + t.Errorf("expected build-time curl+ca-certs install in bins stage; got:\n%s", frags.PreAppStages) + } + + // App-stage gets an explicit per-binary COPY — not a wholesale + // /usr/local/bin/ directory copy. + foundCopy := false + for _, c := range frags.BinCopies { + if strings.Contains(c, "COPY --from=bins /usr/local/bin/kubectl /usr/local/bin/kubectl") { + foundCopy = true + } + if strings.Contains(c, "/usr/local/bin/ ") { + t.Errorf("BinCopies must not emit wholesale-directory COPYs (issue #149); got %q", c) + } + } + if !foundCopy { + t.Errorf("BinCopies missing explicit COPY for kubectl; got %v", frags.BinCopies) } } func TestGenerateDockerfile_CustomBaseImage(t *testing.T) { manifest := &BinManifest{ Requirements: []contract.BinRequirement{ - {Name: "jq"}, + {Name: "kubectl"}, }, } cfg := types.PackageConfig{BaseImage: "ubuntu:24.04"} - content, _, err := GenerateDockerfile(manifest, cfg, false, false) + frags, _, err := GenerateDockerfile(manifest, cfg, false, false) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(content, "FROM ubuntu:24.04") { - t.Error("expected custom base image ubuntu:24.04") + if !strings.Contains(frags.PreAppStages, "FROM ubuntu:24.04") { + t.Errorf("expected custom base image ubuntu:24.04 in bins stage; got:\n%s", frags.PreAppStages) } } -func TestGenerateDockerfile_Heavy(t *testing.T) { +// TestGenerateDockerfile_ImageCopy_SkipsBinsStage confirms image-copy +// binaries (the heavy ones like playwright) come directly from their +// companion `bin-` stage to the app stage, with no bins-stage +// hop. Pre-fix the app stage's wholesale `/usr/local/bin/` copy from +// bins meant playwright had to be routed through bins first. +func TestGenerateDockerfile_ImageCopy_SkipsBinsStage(t *testing.T) { manifest := &BinManifest{ Requirements: []contract.BinRequirement{ {Name: "playwright"}, }, } - content, _, err := GenerateDockerfile(manifest, types.PackageConfig{}, false, false) + frags, _, err := GenerateDockerfile(manifest, types.PackageConfig{}, false, false) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(content, "FROM mcr.microsoft.com/playwright") { - t.Error("expected playwright companion image stage") + if !strings.Contains(frags.PreAppStages, "FROM mcr.microsoft.com/playwright") { + t.Errorf("expected playwright companion image stage in PreAppStages; got:\n%s", frags.PreAppStages) } - if !strings.Contains(content, "COPY --from=bin-playwright") { - t.Error("expected COPY from companion stage") + // Image-copy bins should NOT trigger a shared bins stage. + if strings.Contains(frags.PreAppStages, "AS bins\n") { + t.Errorf("image-copy-only manifest should not emit a shared bins stage; got:\n%s", frags.PreAppStages) + } + + foundCopy := false + for _, c := range frags.BinCopies { + if strings.Contains(c, "COPY --from=bin-playwright") { + foundCopy = true + } + } + if !foundCopy { + t.Errorf("BinCopies missing explicit COPY from playwright companion stage; got %v", frags.BinCopies) } } -func TestGenerateDockerfile_LocalFile(t *testing.T) { +func TestGenerateDockerfile_LocalFile_EmittedInAppStageOnly(t *testing.T) { manifest := &BinManifest{ Requirements: []contract.BinRequirement{ {Name: "forge"}, @@ -136,16 +201,30 @@ func TestGenerateDockerfile_LocalFile(t *testing.T) { }, } - content, _, err := GenerateDockerfile(manifest, cfg, false, false) + frags, _, err := GenerateDockerfile(manifest, cfg, false, false) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(content, "COPY .local-bins/forge /usr/local/bin/forge") { - t.Errorf("expected COPY .local-bins/forge instruction, got:\n%s", content) + // Local-file copies don't need a bins stage — they're COPY-from-context. + if strings.Contains(frags.PreAppStages, "AS bins") { + t.Errorf("local-file-only manifest should not emit a bins stage; got:\n%s", frags.PreAppStages) + } + + foundCopy, foundChmod := false, false + for _, c := range frags.BinCopies { + if strings.Contains(c, "COPY .local-bins/forge /usr/local/bin/forge") { + foundCopy = true + } + if strings.Contains(c, "RUN chmod 0755 /usr/local/bin/forge") { + foundChmod = true + } + } + if !foundCopy { + t.Errorf("BinCopies missing local-file COPY; got %v", frags.BinCopies) } - if !strings.Contains(content, "RUN chmod 0755 /usr/local/bin/forge") { - t.Errorf("expected chmod instruction, got:\n%s", content) + if !foundChmod { + t.Errorf("BinCopies missing local-file chmod; got %v", frags.BinCopies) } } @@ -157,13 +236,18 @@ func TestGenerateDockerfile_AlpineBlockedByUbuntu(t *testing.T) { }, } - content, warnings, err := GenerateDockerfile(manifest, types.PackageConfig{}, true, false) + frags, warnings, err := GenerateDockerfile(manifest, types.PackageConfig{}, true, false) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(content, "FROM debian:bookworm-slim") { - t.Error("expected fallback to debian when alpine blocked") + // PreAppStages is the only place a base image appears for this + // manifest (jq → runtime apt, playwright → companion image stage). + // The companion stage uses its own upstream image; the runtime + // base is picked by the caller from the application stage's FROM + // directive (not visible here). + if !strings.Contains(frags.PreAppStages, "FROM mcr.microsoft.com/playwright") { + t.Errorf("expected playwright companion image stage; got:\n%s", frags.PreAppStages) } hasAlpineWarning := false @@ -176,3 +260,49 @@ func TestGenerateDockerfile_AlpineBlockedByUbuntu(t *testing.T) { t.Error("expected warning about alpine being blocked") } } + +// TestGenerateDockerfile_MixedAptAndDirectURL is the bundled +// code-review skill's shape: curl/git/jq via apt + gh via direct URL. +// Pre-fix the apt installs went through the bins stage and were lost +// when the app stage only copied /usr/local/bin/. Post-fix the apt +// installs run in the app stage AND there's a per-binary COPY for gh. +func TestGenerateDockerfile_MixedAptAndDirectURL(t *testing.T) { + manifest := &BinManifest{ + Requirements: []contract.BinRequirement{ + {Name: "curl"}, + {Name: "git"}, + {Name: "jq"}, + {Name: "gh"}, // registry-known → direct URL + }, + } + + frags, _, err := GenerateDockerfile(manifest, types.PackageConfig{}, false, false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // All three apt bins must show up as runtime packages. + wantApt := map[string]bool{"curl": false, "git": false, "jq": false} + for _, p := range frags.RuntimeAptPackages { + if _, ok := wantApt[p]; ok { + wantApt[p] = true + } + } + for pkg, found := range wantApt { + if !found { + t.Errorf("RuntimeAptPackages missing %q (issue #149 — these would have been silently dropped pre-fix); got %v", + pkg, frags.RuntimeAptPackages) + } + } + + // gh must have an explicit per-binary COPY from the bins stage. + foundGH := false + for _, c := range frags.BinCopies { + if strings.Contains(c, "COPY --from=bins /usr/local/bin/gh /usr/local/bin/gh") { + foundGH = true + } + } + if !foundGH { + t.Errorf("BinCopies missing explicit COPY for gh; got %v", frags.BinCopies) + } +} From 65673c232415a31d118bc504a93c3c273668bad2 Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 10 Jun 2026 20:26:30 -0400 Subject: [PATCH 3/3] =?UTF-8?q?docs(core-concepts):=20binary-dependencies?= =?UTF-8?q?=20=E2=80=94=20registry,=20resolution,=20install=20methods,=20h?= =?UTF-8?q?ow=20to=20add=20a=20bin?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a dedicated reference for the bin pipeline so contributors and operators don't have to read forge-core/packaging/ to know how to declare a new tool in a SKILL.md or override one at the project level. Covers: - The four-source priority walker (local-file → skill-local → forge.yaml override → registry → fallback), with the actual precedence implemented in bin_classifier.go. - The six install methods (apt, apk, direct-URL, custom-run, image-copy, local-file) and which Dockerfile slot each routes through after the issue #149 split. - The four ways to add a binary: use a registry entry, declare inline in SKILL.md, override in forge.yaml, or supply a local file via --local-bin. Each path with a copy-pasteable example. - A registry-entry field reference for contributors adding to image-registry.yaml. - A quick decision tree and a sample of the post-#149 generated Dockerfile so readers see what `forge build` actually emits. Cross-links added in: - docs/core-concepts/skill-md-format.md — the bins: bullet now points here for resolution rules. - docs/reference/forge-yaml-schema.md — package.bin_overrides points here for source priority. - docs/reference/cli-reference.md — --local-bin rows in both `forge build` and `forge package` point at the local-file section. - docs/deployment/docker.md — bin pipeline reference under "Building Agent Container Images". - README.md — Core Concepts table gains a row. sync-docs mapping table extended so future changes to forge-skills/registry/image-registry.yaml, forge-core/packaging/, forge-cli/templates/Dockerfile.tmpl, or forge-cli/build/dockerfile_stage.go refresh this doc. --- .claude/commands/sync-docs.md | 2 + README.md | 1 + docs/core-concepts/binary-dependencies.md | 226 ++++++++++++++++++++++ docs/core-concepts/skill-md-format.md | 2 +- docs/deployment/docker.md | 2 + docs/reference/cli-reference.md | 4 +- docs/reference/forge-yaml-schema.md | 4 +- 7 files changed, 237 insertions(+), 4 deletions(-) create mode 100644 docs/core-concepts/binary-dependencies.md diff --git a/.claude/commands/sync-docs.md b/.claude/commands/sync-docs.md index 3a5a8a0..bfb0212 100644 --- a/.claude/commands/sync-docs.md +++ b/.claude/commands/sync-docs.md @@ -31,6 +31,8 @@ After feature work, update the affected documentation to reflect code changes. | `forge-plugins/` | `docs/core-concepts/channels.md`, `docs/reference/framework-plugins.md` | | `forge-ui/` | `docs/reference/web-dashboard.md` | | `forge-skills/` | `docs/skills/writing-custom-skills.md`, `docs/skills/contributing-a-skill.md` | + | `forge-skills/registry/image-registry.yaml`, `forge-core/packaging/` | `docs/core-concepts/binary-dependencies.md` — refresh the registry contents, install methods, or Dockerfile shape sections; keep the source-file list at the bottom in sync with the actual files touched | + | `forge-cli/templates/Dockerfile.tmpl`, `forge-cli/build/dockerfile_stage.go` | `docs/core-concepts/binary-dependencies.md` (image-shape section), `docs/deployment/docker.md` | | `forge-core/types/` / `forge.yaml` | `docs/reference/forge-yaml-schema.md` | | `CHANGELOG.md` | (rendered into release notes; no per-doc sync needed) | | Any of `forge-core/**`, `forge-cli/**`, `forge-ui/**`, `forge-plugins/**`, `forge-skills/**`, `docs/**`, `CHANGELOG.md`, `forge.yaml` schema | `.claude/skills/forge.md` — refresh the affected section(s) of the comprehensive knowledge skill. Sweep the specific section that maps to the changed area; don't rewrite the whole file. Keep the table-of-contents anchors in sync with the section headings. | diff --git a/README.md b/README.md index 92a913d..2ee80ed 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,7 @@ You write a `SKILL.md`. Forge compiles it into a secure, runnable agent with egr | Document | Description | |----------|-------------| | [Skills](docs/skills/writing-custom-skills.md) | Skill definitions, registry, and compilation | +| [Binary Dependencies](docs/core-concepts/binary-dependencies.md) | How `forge build` resolves, installs, and places skill-declared binaries | | [Tools](docs/core-concepts/tools-and-builtins.md) | Built-in tools, adapters, and custom tools | | [Runtime](docs/core-concepts/runtime-engine.md) | LLM providers, fallback chains, running modes | | [Memory](docs/core-concepts/memory-system.md) | Session persistence and long-term memory | diff --git a/docs/core-concepts/binary-dependencies.md b/docs/core-concepts/binary-dependencies.md new file mode 100644 index 0000000..b46e876 --- /dev/null +++ b/docs/core-concepts/binary-dependencies.md @@ -0,0 +1,226 @@ +--- +title: "Binary Dependencies" +description: "How forge build resolves, installs, and places skill-declared binaries in the runtime container image." +order: 8 +--- + +## Binary Dependencies + +Skills declare the binaries they need (`curl`, `gh`, `kubectl`, …) in their `SKILL.md` frontmatter. `forge build` resolves each one against a layered set of sources, classifies it by install method, and emits the right Dockerfile instructions so the binary lands at a path the runtime can call. This page documents the resolution pipeline and the four ways to add a binary. + +For the SKILL.md frontmatter contract itself, see [SKILL.md Format](skill-md-format.md). For the build pipeline that consumes this resolution, see [Architecture](how-forge-works.md). + +## Sources, in priority order + +The classifier walks four sources for each declared binary and takes the **first hit**. Implemented in `forge-core/packaging/bin_classifier.go`. + +| Priority | Source | Where it's declared | Use case | +|---|---|---|---| +| 0 | **Local file override** | `forge build --local-bin =/abs/path` flag, or `package.bin_overrides..local: /abs/path` in `forge.yaml` | Pinning to an internal build, dev iteration, air-gapped installs | +| 1 | **Skill-local override** | `SKILL.md` frontmatter — set `url:`, `run:`, `apt:`, `apk:` on the bin entry itself | Skill needs a bin not in the registry; install metadata travels with the skill | +| 2 | **`forge.yaml` override** | `package.bin_overrides.` with `apt:`, `apk:`, `url:`, `run:`, `dest:`, `chmod:` | Project-level repinning across all skills (e.g. one internal mirror for `kubectl`) | +| 3 | **Registry lookup** | `forge-skills/registry/image-registry.yaml` — match by binary name | The 70+ pre-vetted bins shipped with `forge` | +| 4 | **Fallback** | None — assumes the apt/apk package name equals the binary name | Emits a build-time warning; works for common Debian package names | + +The registry is the embedded YAML compiled into the `forge` binary. It groups bins by category: core CLI tools (`jq`, `curl`, `git`, `tar`, …), cloud CLIs (`kubectl`, `gh`, `aws`, `gcloud`, `az`, `terraform`, …), databases (`psql`, `mysql`, `redis-cli`, …), languages and runtimes (`node`, `go`, `bun`, `deno`, …), networking (`httpie`, `nmap`, `dig`, …), and heavy/companion-image bins (`playwright`, `chromium`, …). + +## Install methods + +The classifier returns one of six install methods per binary. Each routes through a different Dockerfile slot. See `forge-core/packaging/dockerfile_generator.go` for the emitter. + +| Method | Where it runs in the Dockerfile | When the classifier picks it | +|---|---|---| +| `apt` | Application stage: `RUN apt-get install -y --no-install-recommends ` | Debian/Ubuntu, registry entry has `apt:`, or the fallback heuristic | +| `apk` | Application stage: `RUN apk add --no-cache ` | Alpine, registry entry has `apk:` | +| `direct-URL` | Bins stage download + per-binary `COPY --from=bins ` into app stage | Registry entry has `url:`, no `run:` block | +| `custom-run` | Bins stage executes a script of `RUN ` directives + per-binary `COPY --from=bins` into app stage | Registry entry has `run:` (multi-step install — tar/unzip/configure) | +| `image-copy` | Companion `FROM AS bin-` stage + per-binary `COPY --from=bin-` directly into app stage | Registry entry has `heavy: true` + `image:` (browsers, ML frameworks) | +| `local-file` | Application stage: `COPY .local-bins/ ` + `RUN chmod` | Set via `--local-bin` flag or `package.bin_overrides..local` | + +**Why apt installs run in the app stage** (issue #149): apt-installed binaries land at `/usr/bin/` on Debian with transitive deps in `/usr/lib/`, `/etc/`. Routing them through a separate bins stage and copying just `/usr/bin/` would break them — dependent libs and config files wouldn't come along. Running the apt install in the application stage lets apt's own dependency resolution pull everything in correctly. + +**Why direct-URL / custom-run / image-copy use the bins stage**: these methods produce static, single-file binaries (or self-contained directories). They can be copied with one per-binary `COPY` and don't need package-manager dependency resolution. Keeping the bins stage scoped to these methods means the application image stays small. + +## The four ways to add a binary + +### 1. Use an existing registry entry + +The fastest path. List the bin name in your SKILL.md frontmatter: + +```yaml +--- +name: my-skill +metadata: + forge: + requires: + bins: + - jq # registry → apt: jq, apk: jq + - curl + - kubectl # registry → direct URL download, pinned version +--- +``` + +Discover what's already in the registry by reading `forge-skills/registry/image-registry.yaml`, or run `forge skills add ` to import a vetted skill that already declares its bins. + +### 2. Declare an unknown binary inline in SKILL.md + +If the bin you need isn't in the registry, give the classifier enough info inline. The mapping form replaces the scalar form: + +```yaml +metadata: + forge: + requires: + bins: + # apt-installable, package name differs from bin name + - name: my-cli + apt: my-cli-debian-package + apk: my-cli-alpine-package + + # Direct URL download (static binary) + - name: vault + url: "https://releases.hashicorp.com/vault/1.18.0/vault_1.18.0_linux_amd64.zip" + dest: /usr/local/bin/vault + chmod: "0755" + + # Multi-step install (custom RUN script) + - name: cosign + run: + - "curl -fsSL https://github.com/sigstore/cosign/releases/download/v2.4.0/cosign-linux-amd64 -o /usr/local/bin/cosign" + - "chmod 0755 /usr/local/bin/cosign" +``` + +This keeps install metadata with the skill that needs it. Same skill works across projects without a forge.yaml change. + +### 3. Override a registry entry at the project level + +When every skill in your project should use a different install method for the same bin (internal mirror, pinned version, custom build), put it in `forge.yaml`: + +```yaml +package: + bin_overrides: + kubectl: # repin to internal mirror + url: "https://internal-mirror.example.com/kubectl-1.30.5-linux-amd64" + dest: /usr/local/bin/kubectl + chmod: "0755" + + redis-cli: # use a specific package version + run: + - "apk add --no-cache redis-tools=7.2-r0" + + forge: # point at a locally-built binary + local: ./bin/forge-linux-amd64 +``` + +A `forge.yaml` override beats the registry but loses to a skill-local override (priority 1). See [`forge.yaml` schema](../reference/forge-yaml-schema.md#package) for the full `package.bin_overrides` field reference. + +### 4. Use a local binary file (dev / testing / air-gap) + +Quickest iteration loop — no `forge.yaml` edit needed: + +```sh +forge build --local-bin forge=/Users/you/go/bin/forge \ + --local-bin my-tool=/tmp/my-tool-linux-amd64 +``` + +The file is copied into `.forge-output/.local-bins/` and the Dockerfile emits a `COPY .local-bins/ /usr/local/bin/`. Repeatable for multiple bins. See the [`forge build --local-bin` flag](../reference/cli-reference.md#forge-build) reference. + +### Adding to the registry permanently + +If you maintain a Forge fork or want to upstream a new bin, edit `forge-skills/registry/image-registry.yaml` and submit a PR. The simplest possible entry just maps to apt/apk package names: + +```yaml +bins: + cosign: + url: "https://github.com/sigstore/cosign/releases/download/v{{.Version}}/cosign-linux-amd64" + default_version: "2.4.0" + chmod: "0755" +``` + +Available fields: + +| Field | Purpose | +|---|---| +| `apt` | Debian/Ubuntu package name (defaults to bin name) | +| `apk` | Alpine package name | +| `url` | Direct download URL — supports `{{.Version}}` template | +| `default_version` | Used when the skill doesn't specify `version:` | +| `dest` | Install path — default `/usr/local/bin/` | +| `chmod` | Permission bits — default `"0755"` | +| `heavy` | When `true`, pull from a companion Docker image instead of apt/url | +| `image` | Companion Docker image template (with `heavy: true`) | +| `requires_ubuntu` | Forces Debian/Ubuntu base image; incompatible with Alpine | +| `requires_first` | Other bins that must install first (e.g. `unzip` before `terraform`) | +| `run` | Custom `RUN` lines — replaces apt/url emission entirely; use for multi-step installs | + +## Quick decision tree + +``` +Need a binary in the container? +│ +├─ Is it in image-registry.yaml? +│ └─ Yes → list the name in SKILL.md `requires.bins`. Done. +│ +├─ Is it a standard apt/apk package whose name matches? +│ └─ Yes → list the name (fallback handles it; expect a "not found in registry" warning). +│ +├─ Is it a static binary from upstream? +│ └─ Use `url:` inline in SKILL.md, or add a registry entry. +│ +├─ Does install need multiple steps (tar/unzip/configure)? +│ └─ Use `run:` (custom-run) inline or in registry. +│ +├─ Is it heavy / shipped as a Docker image (browser, ML model)? +│ └─ Registry-level `heavy: true` + `image: `. +│ +└─ Pinned internal build / local dev / air-gap? + └─ `package.bin_overrides..local:` in forge.yaml, or `forge build --local-bin`. +``` + +## What ends up in the runtime image + +After [PR #150](https://github.com/initializ/forge/pull/150) (issue #149), the generated Dockerfile is intent-explicit per binary: + +```dockerfile +# --- Binary installation stages (auto-generated) --- +FROM debian:bookworm-slim AS bins +RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates && rm -rf /var/lib/apt/lists/* +RUN curl -fsSL https://github.com/cli/cli/releases/.../gh_2.60.0_linux_amd64.tar.gz | tar xz -C /tmp +RUN mv /tmp/gh_2.60.0_linux_amd64/bin/gh /usr/local/bin/gh +RUN chmod 0755 /usr/local/bin/gh + +# --- Application stage --- +FROM debian:bookworm-slim +WORKDIR /app +COPY --from=bins /usr/local/bin/gh /usr/local/bin/gh # ← per-binary, not /usr/local/bin/ +COPY . . +RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates curl git jq && rm -rf /var/lib/apt/lists/* +# ... forge framework install, EXPOSE, ENTRYPOINT +``` + +Reading conventions: + +- The bins stage's apt install is build-time only — its `curl` and `ca-certificates` never reach the application image. They exist to let the bins stage download direct-URL binaries. +- Each binary the application stage needs has its own `COPY` line (per-binary, not wholesale `/usr/local/bin/`). New bins reaching the app stage land as new `COPY` lines, not hidden inside a directory copy. +- The application stage's apt install line carries both `ca-certificates` (always needed for TLS) and every runtime apt package the agent's skills declared. + +See [Docker Deployment](../deployment/docker.md) for the operator-facing build / run / push workflow. + +## Cross-references + +- [SKILL.md Format § Frontmatter](skill-md-format.md#yaml-frontmatter) — the `metadata.forge.requires.bins` block +- [`forge.yaml` schema § `package`](../reference/forge-yaml-schema.md#package) — `bin_overrides` field reference +- [`forge build` § Flags](../reference/cli-reference.md#forge-build) — `--local-bin` flag +- [Contributing a Skill § Binary dependencies](../skills/contributing-a-skill.md) — when bundling skills with the runtime +- [Writing Custom Skills](../skills/writing-custom-skills.md) — end-to-end skill authoring +- [Docker Deployment](../deployment/docker.md) — how the built image is run + +## Source files + +- `forge-skills/registry/image-registry.yaml` — the embedded binary registry +- `forge-skills/registry/registry.go` — registry loader +- `forge-core/packaging/bin_classifier.go` — source-priority walker + classifier +- `forge-core/packaging/dockerfile_generator.go` — emits Dockerfile fragments per install method +- `forge-cli/templates/Dockerfile.tmpl` — application-stage template (consumes the fragments) +- `forge-cli/build/dockerfile_stage.go` — wires the generator output into the build pipeline +- `forge-skills/contract/types.go` — `BinRequirement` (the SKILL.md frontmatter shape) +- `forge-core/types/config.go` — `BinOverride` (the `forge.yaml` shape) diff --git a/docs/core-concepts/skill-md-format.md b/docs/core-concepts/skill-md-format.md index 02c5f36..6959f36 100644 --- a/docs/core-concepts/skill-md-format.md +++ b/docs/core-concepts/skill-md-format.md @@ -59,7 +59,7 @@ Top-level fields: The `metadata.forge.requires` block declares runtime dependencies: -- **`bins`** — Binary dependencies that must be in `$PATH` at runtime +- **`bins`** — Binary dependencies that must be in `$PATH` at runtime. Each entry is either a scalar name (matched against the embedded registry) or a mapping with its own install method (`url:`, `run:`, `apt:`, `apk:`). See [Binary Dependencies](binary-dependencies.md) for the resolution pipeline, install methods, and the four ways to add a binary. - **`env.required`** — Environment variables that must be set - **`env.one_of`** — At least one of these environment variables must be set - **`env.optional`** — Optional environment variables for extended functionality diff --git a/docs/deployment/docker.md b/docs/deployment/docker.md index 5023f83..431377f 100644 --- a/docs/deployment/docker.md +++ b/docs/deployment/docker.md @@ -48,6 +48,8 @@ forge export --pretty --include-schemas `forge package` generates a Dockerfile, Kubernetes manifests, and NetworkPolicy. Use `--prod` to strip dev tools and enforce strict egress. Use `--verify` to smoke-test the built container. +The Dockerfile's binary install pipeline — what gets pulled from where, how to add a new binary, and what ends up in the runtime image — is documented in [Binary Dependencies](../core-concepts/binary-dependencies.md). + ## Docker Compose ```bash diff --git a/docs/reference/cli-reference.md b/docs/reference/cli-reference.md index b5a2248..f7ad0e0 100644 --- a/docs/reference/cli-reference.md +++ b/docs/reference/cli-reference.md @@ -142,7 +142,7 @@ Uses global `--config` and `--output-dir` flags. Output is written to `.forge-ou | `--signing-key` | | Path to Ed25519 private key for signing build output | | `--slim` | `false` | Minimize image size (skip heavy/optional binaries) | | `--alpine` | `false` | Prefer Alpine base image | -| `--local-bin` | | Local binary override as `name=/path/to/file` (repeatable) | +| `--local-bin` | | Local binary override as `name=/path/to/file` (repeatable). See [Binary Dependencies § Use a local binary file](../core-concepts/binary-dependencies.md#4-use-a-local-binary-file-dev--testing--air-gap). | | `--policy` | | Path to a YAML `SecurityPolicy` file for the build's `security-analysis` stage (overrides `forge.yaml security.policy_path` and the builtin `DefaultPolicy`). Same schema as `forge skills audit --policy`. See [Skills CLI / Security Audit](../skills/skills-cli.md#security-audit). | ### Examples @@ -372,7 +372,7 @@ forge package [flags] | `--with-channels` | `false` | Generate docker-compose.yaml with channel adapters | | `--slim` | `false` | Minimize image size (skip heavy/optional binaries) | | `--alpine` | `false` | Prefer Alpine base image | -| `--local-bin` | | Local binary override as `name=/path/to/file` (repeatable) | +| `--local-bin` | | Local binary override as `name=/path/to/file` (repeatable). See [Binary Dependencies § Use a local binary file](../core-concepts/binary-dependencies.md#4-use-a-local-binary-file-dev--testing--air-gap). | ### Examples diff --git a/docs/reference/forge-yaml-schema.md b/docs/reference/forge-yaml-schema.md index f3e0b69..90ba3d7 100644 --- a/docs/reference/forge-yaml-schema.md +++ b/docs/reference/forge-yaml-schema.md @@ -63,7 +63,9 @@ server: # A2A server tuning (optional) package: alpine: false # Prefer Alpine base image slim: false # Minimize image size - bin_overrides: # Per-binary install overrides + bin_overrides: # Per-binary install overrides. See + # ../core-concepts/binary-dependencies.md + # for the resolution pipeline and source priority. forge: local: "/path/to/linux/forge" # Host path to local binary file jq: