From 8ec0c16d93576cd98088962203cfceae1e8eb668 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Fri, 21 Aug 2026 14:05:51 -0400 Subject: [PATCH 1/2] feat(builder): add ValidateBuildRequestOutputDirectory guard (#333) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expose a shared guard that enforces the BuildRequest.output_directory contract — when set, the destination must be absolute; empty selects the legacy in-agent build. The guard lives next to the recipe contract in core (where the proto is defined) so the CLI, which populates the field in a separate repo, imports and calls it before sending a BuildRequest. The invariant cannot be enforced in BuildDockerBuildPlan: its tree walk is relative-safe, so an IsAbs guard there would reject valid callers rather than catch the real failure at the point the field is set. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Fe7p9irTPJPkQBcpRVYJzg --- agents/services/docker_recipe.go | 20 ++++++++++++++++++++ agents/services/docker_recipe_test.go | 15 +++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/agents/services/docker_recipe.go b/agents/services/docker_recipe.go index 659ba0a7..d509cb4a 100644 --- a/agents/services/docker_recipe.go +++ b/agents/services/docker_recipe.go @@ -15,6 +15,26 @@ import ( // validates before building from an emitted plan. const DockerBuildRecipeContractVersion = "codefly.dev/docker-build-recipe/v1" +// ValidateBuildRequestOutputDirectory enforces the BuildRequest.output_directory +// contract at the boundary that populates it: when set, the destination must be +// an absolute path the caller owns. Empty is valid and selects the legacy +// in-agent build. The CLI resolves its destination to absolute and calls this +// before sending a BuildRequest, so a relative path is rejected with a clear +// error instead of being silently resolved against whatever working directory +// the agent happens to run in. BuildDockerBuildPlan cannot enforce this — its +// tree walk is relative-safe, so a guard there would reject valid callers rather +// than catch the real failure at the point the field is set. +func ValidateBuildRequestOutputDirectory(req *builderv0.BuildRequest) error { + dir := req.GetOutputDirectory() + if dir == "" { + return nil + } + if !filepath.IsAbs(dir) { + return fmt.Errorf("BuildRequest.output_directory must be absolute, got %q", dir) + } + return nil +} + // BuildDockerBuildPlan inventories the recipe tree an agent wrote to destination // and returns a build plan: the ordered recipes plus the canonical sorted file // inventory with per-file sha256 digests and an aggregate digest that is a diff --git a/agents/services/docker_recipe_test.go b/agents/services/docker_recipe_test.go index fc49127a..d33f2c9e 100644 --- a/agents/services/docker_recipe_test.go +++ b/agents/services/docker_recipe_test.go @@ -61,6 +61,21 @@ func TestBuildDockerBuildPlanInventoriesRecipeTree(t *testing.T) { require.Error(t, VerifyDockerBuildPlan(destination, plan)) } +func TestValidateBuildRequestOutputDirectory(t *testing.T) { + // Empty selects the legacy in-agent build and is valid. + require.NoError(t, ValidateBuildRequestOutputDirectory(&builderv0.BuildRequest{})) + + // An absolute destination honors the contract. + require.NoError(t, ValidateBuildRequestOutputDirectory(&builderv0.BuildRequest{ + OutputDirectory: filepath.Join(t.TempDir(), "recipes"), + })) + + // A relative destination is rejected at the boundary that populates the field. + err := ValidateBuildRequestOutputDirectory(&builderv0.BuildRequest{OutputDirectory: "recipes/out"}) + require.Error(t, err) + require.Contains(t, err.Error(), "absolute") +} + func TestBuildDockerBuildPlanDigestChangesWithContent(t *testing.T) { destination := t.TempDir() dockerfile := filepath.Join(destination, "Dockerfile") From 88807422a71487b01dd8c507afd4ede5eb74ed1f Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Fri, 21 Aug 2026 14:19:45 -0400 Subject: [PATCH 2/2] fix(builder): enforce output_directory at the DockerBuildRequest boundary (#333) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior change only exported ValidateBuildRequestOutputDirectory; nothing in core called it, so merging enforced nothing and closing #333 would have declared victory before any code path guarded the invariant. A relative output_directory would still sail through: the agent resolves it against its own working directory while the caller expects an absolute location it owns, so the recipe handshake breaks silently with no error. Wire the guard into BuilderWrapper.DockerBuildRequest — the universal chokepoint every agent runner routes a BuildRequest through, which already validates the build-context kind. It is BuildRequest-specific, so it rejects only contract-violating callers (relative, non-empty), never a valid one; this refutes the "wrong layer" objection that applies to the generic BuildDockerBuildPlan tree walk. Enforcement is now real on every agent build. Also pin the nil-request boundary case in the validator test. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Fe7p9irTPJPkQBcpRVYJzg --- agents/services/base_builder.go | 3 +++ agents/services/docker_recipe.go | 19 ++++++++------- agents/services/docker_recipe_test.go | 34 +++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/agents/services/base_builder.go b/agents/services/base_builder.go index 34303ab0..dacedaac 100644 --- a/agents/services/base_builder.go +++ b/agents/services/base_builder.go @@ -979,6 +979,9 @@ func (s *BuilderWrapper) LogDeployRequest(req *builderv0.DeploymentRequest, log } func (s *BuilderWrapper) DockerBuildRequest(_ context.Context, req *builderv0.BuildRequest) (*builderv0.DockerBuildContext, error) { + if err := ValidateBuildRequestOutputDirectory(req); err != nil { + return nil, s.Wool.Wrapf(err, "cannot build") + } switch v := req.BuildContext.Kind.(type) { case *builderv0.BuildContext_DockerBuildContext: return v.DockerBuildContext, nil diff --git a/agents/services/docker_recipe.go b/agents/services/docker_recipe.go index d509cb4a..cef49488 100644 --- a/agents/services/docker_recipe.go +++ b/agents/services/docker_recipe.go @@ -16,14 +16,17 @@ import ( const DockerBuildRecipeContractVersion = "codefly.dev/docker-build-recipe/v1" // ValidateBuildRequestOutputDirectory enforces the BuildRequest.output_directory -// contract at the boundary that populates it: when set, the destination must be -// an absolute path the caller owns. Empty is valid and selects the legacy -// in-agent build. The CLI resolves its destination to absolute and calls this -// before sending a BuildRequest, so a relative path is rejected with a clear -// error instead of being silently resolved against whatever working directory -// the agent happens to run in. BuildDockerBuildPlan cannot enforce this — its -// tree walk is relative-safe, so a guard there would reject valid callers rather -// than catch the real failure at the point the field is set. +// contract: when set, the destination must be an absolute path the caller owns. +// Empty is valid and selects the legacy in-agent build. A relative path is +// rejected with a clear error rather than silently resolved against whatever +// working directory the agent happens to run in — the agent and the caller would +// otherwise resolve it against different directories and the recipe handshake +// would break with no error at all. DockerBuildRequest calls this so every agent +// build enforces the invariant at the boundary where the request enters core; +// the CLI resolves its destination to absolute and can call it before sending. +// BuildDockerBuildPlan does not enforce it — it takes a bare destination and its +// tree walk is relative-safe, so a guard there would reject valid callers of a +// generic helper rather than catch the contract violation at the request boundary. func ValidateBuildRequestOutputDirectory(req *builderv0.BuildRequest) error { dir := req.GetOutputDirectory() if dir == "" { diff --git a/agents/services/docker_recipe_test.go b/agents/services/docker_recipe_test.go index d33f2c9e..5d7d2de4 100644 --- a/agents/services/docker_recipe_test.go +++ b/agents/services/docker_recipe_test.go @@ -1,6 +1,7 @@ package services import ( + "context" "crypto/sha256" "encoding/hex" "os" @@ -8,6 +9,7 @@ import ( "testing" builderv0 "github.com/codefly-dev/core/generated/go/codefly/services/builder/v0" + "github.com/codefly-dev/core/wool" "github.com/stretchr/testify/require" ) @@ -62,6 +64,9 @@ func TestBuildDockerBuildPlanInventoriesRecipeTree(t *testing.T) { } func TestValidateBuildRequestOutputDirectory(t *testing.T) { + // A nil request carries no directory and selects the legacy in-agent build. + require.NoError(t, ValidateBuildRequestOutputDirectory(nil)) + // Empty selects the legacy in-agent build and is valid. require.NoError(t, ValidateBuildRequestOutputDirectory(&builderv0.BuildRequest{})) @@ -76,6 +81,35 @@ func TestValidateBuildRequestOutputDirectory(t *testing.T) { require.Contains(t, err.Error(), "absolute") } +func TestDockerBuildRequestEnforcesAbsoluteOutputDirectory(t *testing.T) { + wrapper := &BuilderWrapper{Base: &Base{Wool: wool.Get(context.Background())}} + dockerContext := &builderv0.BuildContext{ + Kind: &builderv0.BuildContext_DockerBuildContext{DockerBuildContext: &builderv0.DockerBuildContext{}}, + } + + // A relative output_directory is rejected before the build proceeds, so the + // agent never writes recipes where the caller cannot find them. + _, err := wrapper.DockerBuildRequest(context.Background(), &builderv0.BuildRequest{ + BuildContext: dockerContext, + OutputDirectory: "recipes/out", + }) + require.Error(t, err) + require.Contains(t, err.Error(), "absolute") + + // An absolute output_directory passes through to the docker build context. + got, err := wrapper.DockerBuildRequest(context.Background(), &builderv0.BuildRequest{ + BuildContext: dockerContext, + OutputDirectory: filepath.Join(t.TempDir(), "recipes"), + }) + require.NoError(t, err) + require.NotNil(t, got) + + // An empty output_directory (legacy in-agent build) passes through. + got, err = wrapper.DockerBuildRequest(context.Background(), &builderv0.BuildRequest{BuildContext: dockerContext}) + require.NoError(t, err) + require.NotNil(t, got) +} + func TestBuildDockerBuildPlanDigestChangesWithContent(t *testing.T) { destination := t.TempDir() dockerfile := filepath.Join(destination, "Dockerfile")