From 6032921f6ca2f08da976bbb92b85f9a87890da68 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Wed, 28 Oct 2026 12:54:38 -0400 Subject: [PATCH 1/4] feat(configuration): deliver public module artifacts through ordinary loads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Core acquires a bound module artifact per ordinary configuration load; this is the consumer half. Three CLI-owned corrections ride together because they share the Init/Start delivery seam and one of them is a local-startup blocker. Module artifacts. A workspace value may be bound to a file in a composed module, declared in the selected profile. The resolver is supplied wherever a configuration is loaded, so run delivery, checks, render and the remote manager acquire alike. The declared path is opened confined under the selected module, non-blocking, regular-file only, bounded, with same-open-file observations around the read and the selection re-asked afterwards; a CLI-materialized path needs a receipt answering the request the workspace makes now, and the resolved directory must be that receipt's. The delivery check runs on the complete unmerged payload at Init, the Init response, Start and the excluded-root export, with the override map computed once and reused by the request, and on both public typed exports before any filter or conversion. Only a successful Init retains a detached baseline; Start refuses without one rather than handing Core a nil comparison. The merged fixture composition no longer replaces a group a module artifact is bound to. Output records. The raw --output-env batch is held to the grammar the supported reader parses, across every record and before the first write, so a value carrying a line break can no longer smuggle a second record past semantic key matching. One serialization is checked and retained through the write, and the shared writer re-checks supplied and retained records. This surfaces that a structured configuration document was never loadable from such a file: its carrier is a lowercase hex digest the reader refuses. Local runtime image. The agent's advertisement of runtime-init-image/v1, not the cardinality of its build plan, decides whether the run relies on the image handoff — a database agent's sole recipe is its migration image, and reading the runtime role off it built the wrong image and then failed the run for a capability the agent never claimed. Checked before the builder is loaded, and only the absence of the optional capability is waived. Runnable audiences. An explicitly annotated audience may name a host surface the module reaches without being installed as one, so it needs no module contract; a default-derived audience still requires the real principal, and an existing contract still governs destinations and principal equality. A declaration that exists and cannot be read — a dangling in-root symlink included — is never absence. Adopts core v0.19.11-0.20261028153419-de19eba80550 and moves the conformance matrix core line with it. Updates docs/configuration-contract.md, docs/commands.md and docs/runnable.md. Co-Authored-By: Claude Opus 5 --- cmd/doctor_workspace.go | 5 +- cmd/environment/import.go | 3 +- cmd/generate/runnable_bindings.go | 5 + cmd/generate/runnables.go | 94 +++- cmd/generate/runnables_audience_test.go | 113 ++++- docs/commands.md | 35 ++ docs/configuration-contract.md | 65 +++ docs/runnable.md | 40 +- go.mod | 2 +- go.sum | 2 + pkg/cli/output_sink.go | 25 +- pkg/composition/fixtures.go | 3 +- pkg/composition/module_selection.go | 94 ++++ pkg/conformance/matrix.json | 2 +- pkg/environments/environment.go | 20 + pkg/environments/product_secrets.go | 10 +- pkg/gitops/authority.go | 3 +- pkg/gitops/configuration_groups.go | 3 +- pkg/gitops/module_authority.go | 3 +- pkg/gitops/runnable_grants.go | 3 +- pkg/orchestration/disposable_engine_test.go | 16 +- pkg/orchestration/environment.go | 16 +- .../environment_remote_manager.go | 4 + pkg/orchestration/excluded_root_output_env.go | 47 +- pkg/orchestration/hop_configurations.go | 28 +- pkg/orchestration/local_runtime_image.go | 28 ++ pkg/orchestration/local_runtime_image_test.go | 125 ++++- pkg/orchestration/module_artifact.go | 134 ++++++ .../module_artifact_delivery_test.go | 220 +++++++++ pkg/orchestration/module_artifact_test.go | 187 ++++++++ pkg/orchestration/output_records.go | 109 +++++ pkg/orchestration/output_records_test.go | 234 ++++++++++ pkg/orchestration/runner.go | 435 +++++++++++++----- .../runner_accepted_network_mappings_test.go | 10 + pkg/orchestration/runner_output_env_test.go | 20 +- .../runner_reload_addresses_test.go | 9 +- 36 files changed, 1927 insertions(+), 225 deletions(-) create mode 100644 pkg/composition/module_selection.go create mode 100644 pkg/orchestration/module_artifact.go create mode 100644 pkg/orchestration/module_artifact_delivery_test.go create mode 100644 pkg/orchestration/module_artifact_test.go create mode 100644 pkg/orchestration/output_records.go create mode 100644 pkg/orchestration/output_records_test.go diff --git a/cmd/doctor_workspace.go b/cmd/doctor_workspace.go index f88ace6af..fb051bff2 100644 --- a/cmd/doctor_workspace.go +++ b/cmd/doctor_workspace.go @@ -1380,7 +1380,10 @@ func checkServiceConfigurationSources(ctx context.Context, env *environments.Env // module's, is reported and returns nil. Duplicate keys within one // configuration are flagged as for a service directory. func loadWorkspaceConfigurations(ctx context.Context, ws *resources.Workspace, env *resources.Environment, relCfgDir string, report *workspaceReadinessReport) *configurations.WorkspaceConfigurations { - provided, err := configurations.ReadWorkspaceConfigurations(ctx, ws, env) + // The same acquisition a run supplies, so the doctor reports what a run + // would deliver for a bound module artifact rather than core's + // "requires a resolver" refusal. + provided, err := configurations.ReadWorkspaceConfigurations(ctx, ws, env, orchestration.ModuleArtifactResolverFor(ws)) if err != nil { code := codeConfigurationInvalid if errors.Is(err, configurations.ErrConfigurationConflict) { diff --git a/cmd/environment/import.go b/cmd/environment/import.go index de5b855be..f94fb096a 100644 --- a/cmd/environment/import.go +++ b/cmd/environment/import.go @@ -12,6 +12,7 @@ import ( "github.com/codefly-dev/cli/cmd/common" "github.com/codefly-dev/cli/pkg/cli/yamledit" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/resources" "github.com/codefly-dev/core/shared" "github.com/pmezard/go-difflib/difflib" @@ -281,7 +282,7 @@ func runImport(ctx context.Context, opts *importOptions) error { if err := environments.ValidateEnvironmentExclusions(ctx, workspace, selected); err != nil { return fmt.Errorf("product intent: %w", err) } - if err := environments.ValidateSecretPolicy(ctx, workspace, selected, intent.ServiceSecrets); err != nil { + if err := environments.ValidateSecretPolicy(ctx, workspace, selected, intent.ServiceSecrets, orchestration.ModuleArtifactResolverFor(workspace)); err != nil { return fmt.Errorf("product intent: %w", err) } } diff --git a/cmd/generate/runnable_bindings.go b/cmd/generate/runnable_bindings.go index 5daaf89f9..a833ccd37 100644 --- a/cmd/generate/runnable_bindings.go +++ b/cmd/generate/runnable_bindings.go @@ -15,6 +15,7 @@ import ( "github.com/codefly-dev/cli/cmd/common" "github.com/codefly-dev/cli/pkg/cli" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/cli/pkg/remotenetwork" runnablespkg "github.com/codefly-dev/cli/pkg/runnables" "github.com/codefly-dev/core/composition" @@ -655,6 +656,10 @@ func newEndpointResolver(ctx context.Context, workspace *resources.Workspace, en if err != nil { return nil, err } + // The same acquisition every other delivery and check load supplies: a + // bound module artifact reaching this manager must be resolved from the + // selected module, not refused as unresolvable (module_artifact.go). + reader.WithModuleArtifactResolver(orchestration.ModuleArtifactResolverFor(workspace)) manager.WithLoader(reader) if resolver.remote, err = remotenetwork.NewRemoteManager(ctx, manager); err != nil { return nil, err diff --git a/cmd/generate/runnables.go b/cmd/generate/runnables.go index 4ba41af2e..7e029269e 100644 --- a/cmd/generate/runnables.go +++ b/cmd/generate/runnables.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "io/fs" "os" "path/filepath" "slices" @@ -159,6 +160,9 @@ type derivation struct { files map[string][]byte publication *modulecontract.Contract publicationErr error + // publicationAbsent means the module publishes no contract AT ALL, as + // distinct from publishing one that could not be read. + publicationAbsent bool // release is the module's own release version, empty for a module that // declares no package manifest. release string @@ -271,6 +275,13 @@ func deriveRunnablesIn(ctx context.Context, workspaceName, workspaceDir string, // Load the selected owner's declaration once. A module with no operations // still produces an empty index; an operation cannot publish without it. derived.publication, derived.publicationErr = modulecontract.Load(module.Dir()) + // Whether the module HAS a declaration is a different question from + // whether it could be read, and only an existence observation answers it. + // Core's loader reads through os.Root, which reports a dangling in-root + // symlink as fs.ErrNotExist — so the error alone cannot tell "this module + // publishes no contract" from "its contract is a broken link". Core keeps + // owning the content; this is only the entry. + derived.publicationAbsent = derived.publicationErr != nil && !publicationEntryExists(module.Dir()) var endpoints []composition.APIContractEndpoint if catalog != nil { @@ -546,16 +557,60 @@ func (d *derivation) audience() string { // publication principal is independent of the producer checkout's module name; // an explicitly declared operation must agree, never be silently rewritten. func (d *derivation) record(endpoint *composition.APIContractEndpoint, name string, pkg *basev0.RunnablePackage, spec *corerunnable.OperationSpec, origin derivedOperation) error { - if d.publicationErr != nil { - return fmt.Errorf("runnable %s/%s/%s needs its owner's module contract: %w", endpoint.Service, endpoint.Endpoint, name, d.publicationErr) + // An unreadable or malformed contract is never absence. A module that + // authored one and got it wrong must be told so, whatever the operation — + // turning a schema skew, a bad field or a dangling link into "no module + // publication applies" would admit exactly the declarations the strict + // reader exists to refuse. + if d.publicationErr != nil && !d.publicationAbsent { + return fmt.Errorf("runnable %s/%s/%s cannot read its owner's module contract: %w", endpoint.Service, endpoint.Endpoint, name, d.publicationErr) } - if d.publication == nil || !slices.ContainsFunc(d.publication.Destinations, func(destination modulecontract.Destination) bool { - return destination.Service == endpoint.Service && destination.Endpoint == endpoint.Endpoint - }) { - return fmt.Errorf("module contract does not publish runnable destination %s/%s", endpoint.Service, endpoint.Endpoint) - } - if spec.Audience != d.publication.Principal { - return fmt.Errorf("runnable %s/%s/%s audience %q differs from module contract principal %q", endpoint.Service, endpoint.Endpoint, name, spec.Audience, d.publication.Principal) + switch { + case d.publicationErr == nil: + // A real contract exists, so the module's publication applies to every + // operation it serves — including an explicit one. Its destinations are + // the endpoints it exposes, and its principal is the one audience its + // credentials are issued to: an explicit audience that differs from it + // is a conflict in the module's own declarations, not host permission, + // and is refused rather than reinterpreted. + if !slices.ContainsFunc(d.publication.Destinations, func(destination modulecontract.Destination) bool { + return destination.Service == endpoint.Service && destination.Endpoint == endpoint.Endpoint + }) { + return fmt.Errorf("module contract does not publish runnable destination %s/%s", endpoint.Service, endpoint.Endpoint) + } + if spec.Audience != d.publication.Principal { + return fmt.Errorf("runnable %s/%s/%s audience %q differs from module contract principal %q", endpoint.Service, endpoint.Endpoint, name, spec.Audience, d.publication.Principal) + } + case !origin.explicit: + // Absent, and this operation's audience was DERIVED. + // No contract at all, and this operation's audience was DERIVED — core + // defaults it from ServiceOwner.Audience, which this generation fills + // from the module publication. With no publication there is no + // principal to derive from, so the audience would be empty: refuse, + // naming what is missing. + return fmt.Errorf("runnable %s/%s/%s derives its audience by default and so needs its owner's module contract: %w", endpoint.Service, endpoint.Endpoint, name, d.publicationErr) + default: + // No contract, and the method carries the operation option — which is + // what core's OperationSpec.Declared marks: the OPTION's presence, not + // the presence of an audience in it. So the audience is the author's + // own, read from that option rather than derived from any module + // principal: a host surface a module reaches without being installed as + // one. Core does not require it to equal a principal, and requiring a + // contract here demanded a module identity the module does not have. + // + // Core's OperationFromMethod validates the audience before it sets + // Declared, so an annotated method with an empty audience does not + // reach here; the check is kept anyway, because this branch is the one + // place an audience is accepted without a principal to compare it + // against, and it must not be the one that lets an empty one through. + // + // The served-target, scope and lookup checks upstream are unchanged, so + // an unserved method or a missing paired Lookup still refuses, and the + // explicit audience and scopes are carried through exactly as core + // derived them. + if spec.Audience == "" { + return fmt.Errorf("runnable %s/%s/%s carries the operation option but declares no audience, and its owner publishes no module contract", endpoint.Service, endpoint.Endpoint, name) + } } operation := pkg.GetServiceOperations()[0] packageBytes, err := runnablespkg.CanonicalJSON(pkg) @@ -708,3 +763,24 @@ func kebab(name string) string { func methodName(fullMethod string) string { return fullMethod[strings.LastIndex(fullMethod, "/")+1:] } + +// publicationEntryExists reports whether the module directory holds a +// module-contract entry, whatever the state of what it points at. +// +// Lstat, not Stat: a dangling symlink IS a declaration the module authored, +// and following it would report the same "not found" that Core's loader +// already reports — which is exactly the conflation this exists to break. +// Confined through os.Root like every other read of a composed module's tree, +// and any failure to observe is read as "present", so an unobservable entry +// refuses rather than passing as absence. +func publicationEntryExists(moduleDir string) bool { + root, err := os.OpenRoot(moduleDir) + if err != nil { + return true + } + defer func() { _ = root.Close() }() + if _, err := root.Lstat(modulecontract.FileName); err != nil { + return !errors.Is(err, fs.ErrNotExist) + } + return true +} diff --git a/cmd/generate/runnables_audience_test.go b/cmd/generate/runnables_audience_test.go index 7270322a0..075c69b75 100644 --- a/cmd/generate/runnables_audience_test.go +++ b/cmd/generate/runnables_audience_test.go @@ -35,12 +35,12 @@ func TestRunnableAudienceRefusalsLeaveOutputUnchanged(t *testing.T) { edit func(*testing.T, string) want string }{ - {"missing", func(t *testing.T, dir string) { - require.NoError(t, os.Remove(filepath.Join(dir, modulecontract.FileName))) - }, "needs its owner's module contract"}, + // A contract that exists and cannot be read is never absence: a module + // that authored one and got it wrong is told so, whatever kind of + // operation it serves. {"malformed", func(t *testing.T, dir string) { writeFixtureFile(t, filepath.Join(dir, modulecontract.FileName), []byte("schema: codefly/module-contract/v1\nprincipal: documents\nunknown: true\n")) - }, "needs its owner's module contract"}, + }, "cannot read its owner's module contract"}, {"uncovered destination", func(t *testing.T, dir string) { saveRunnablePublication(t, dir, "documents.ingestion", modulecontract.Destination{ID: "other", Service: "runtime-worker", Endpoint: "other", Kind: "module"}) }, "does not publish runnable destination runtime-worker/grpc"}, @@ -54,7 +54,7 @@ func TestRunnableAudienceRefusalsLeaveOutputUnchanged(t *testing.T) { writeFixtureFile(t, outside, data) require.NoError(t, os.Remove(filepath.Join(dir, modulecontract.FileName))) require.NoError(t, os.Symlink(outside, filepath.Join(dir, modulecontract.FileName))) - }, "needs its owner's module contract"}, + }, "cannot read its owner's module contract"}, } { t.Run(test.name, func(t *testing.T) { ctx := context.Background() @@ -80,3 +80,106 @@ func TestRunnableAudienceChecksOpenAPIPublication(t *testing.T) { require.ErrorContains(t, RunnablesCmd.RunE(RunnablesCmd, []string{"releases"}), "differs from module contract principal") require.Equal(t, before, snapshotTree(t, moduleDir)) } + +// TestRunnableExplicitAudienceNeedsNoModuleContract is the SaaS case, and the +// behaviour that replaces a universal gate. +// +// The author annotated the method's audience explicitly (core sets +// OperationSpec.Declared from the method option), so the audience is theirs — +// a host surface the module reaches without being installed as one. Core does +// not require it to equal any module principal, and demanding a module +// contract here demanded an identity the module does not have. Generation +// proceeds and carries the explicit audience, its scopes and its lookup +// through unchanged. +func TestRunnableExplicitAudienceNeedsNoModuleContract(t *testing.T) { + ctx := context.Background() + root, moduleDir := saveRunnableFixture(t, ctx, descriptorSet(t, ingestionFile(conformingOperation())), "0.1.0") + t.Chdir(root) + resetRunnablesFlags(t) + require.NoError(t, os.Remove(filepath.Join(moduleDir, modulecontract.FileName))) + + require.NoError(t, RunnablesCmd.RunE(RunnablesCmd, []string{"documents"})) + + data, err := os.ReadFile(filepath.Join(moduleDir, filepath.FromSlash(runnablespkg.DerivedDir), + "runtime-worker", "grpc", "ApplyText", runnablespkg.OperationFileName)) + require.NoError(t, err) + var operation map[string]any + require.NoError(t, json.Unmarshal(data, &operation)) + require.Equal(t, "documents.ingestion", operation["audience"], "the explicit audience is preserved, not derived") + require.Equal(t, "/documents.ingest.v1.IngestionService/LookupText", operation["lookup_method"], "and its lookup") + require.NotEmpty(t, operation["invoke_scopes"], "and its scopes") + require.NotEmpty(t, operation["lookup_scopes"]) +} + +// TestRunnableDefaultAudienceStillNeedsTheModuleContract keeps the waiver +// narrow. A method with no explicit audience takes core's default from +// ServiceOwner.Audience, which this generation fills from the module +// publication — so with no contract there is no principal to derive from, and +// it refuses by name rather than emitting an empty audience. +func TestRunnableDefaultAudienceStillNeedsTheModuleContract(t *testing.T) { + ctx := context.Background() + file := ingestionFile(conformingOperation(), hopPolicyMethod("CountText", "documents", "read")) + root, moduleDir := saveRunnableFixture(t, ctx, withHopPolicy(t, descriptorSet(t, file)), "0.1.0") + t.Chdir(root) + resetRunnablesFlags(t) + require.NoError(t, RunnablesCmd.RunE(RunnablesCmd, []string{"documents"})) + before := snapshotTree(t, moduleDir) + + require.NoError(t, os.Remove(filepath.Join(moduleDir, modulecontract.FileName))) + err := RunnablesCmd.RunE(RunnablesCmd, []string{"documents"}) + require.ErrorContains(t, err, "derives its audience by default and so needs its owner's module contract") + require.Equal(t, before, snapshotTree(t, moduleDir), "a refusal leaves the output unchanged") +} + +// TestRunnableDanglingModuleContractIsNotAbsence closes the gap between "no +// declaration" and "a declaration that cannot be read". +// +// Core's loader reads through os.Root, and a dangling symlink INSIDE the module +// reports fs.ErrNotExist — the same error as a module that published nothing. +// Treating that as absence would let a broken declaration take the explicit- +// audience path, which is the one path that accepts an audience with no +// principal to check it against. The entry exists, so generation refuses and +// writes nothing. +func TestRunnableDanglingModuleContractIsNotAbsence(t *testing.T) { + ctx := context.Background() + root, moduleDir := saveRunnableFixture(t, ctx, descriptorSet(t, ingestionFile(conformingOperation())), "0.1.0") + t.Chdir(root) + resetRunnablesFlags(t) + require.NoError(t, RunnablesCmd.RunE(RunnablesCmd, []string{"documents"})) + before := snapshotTree(t, moduleDir) + + // A link to a sibling that does not exist: in-root, so os.Root resolves it + // rather than refusing the escape, and finds nothing. + contract := filepath.Join(moduleDir, modulecontract.FileName) + require.NoError(t, os.Remove(contract)) + require.NoError(t, os.Symlink("absent.module.contract.codefly.yaml", contract)) + entry, err := os.Lstat(contract) + require.NoError(t, err, "the declaration entry exists") + require.NotZero(t, entry.Mode()&os.ModeSymlink) + _, statErr := os.Stat(contract) + require.True(t, os.IsNotExist(statErr), "and what it points at does not") + + err = RunnablesCmd.RunE(RunnablesCmd, []string{"documents"}) + require.ErrorContains(t, err, "cannot read its owner's module contract") + require.Equal(t, before, snapshotTree(t, moduleDir), "a refusal leaves the output unchanged") +} + +// TestRunnableAnnotatedMethodWithNoAudienceRefuses pins the floor under the +// explicit branch. Core's OperationFromMethod validates the audience before it +// sets Declared, so this should be unreachable through core — which is the +// point: the branch that accepts an audience without a principal must not be +// the one that accepts an empty one, whatever reaches it. +func TestRunnableAnnotatedMethodWithNoAudienceRefuses(t *testing.T) { + ctx := context.Background() + operation := conformingOperation() + operation.Audience = "" + root, moduleDir := saveRunnableFixture(t, ctx, descriptorSet(t, ingestionFile(operation)), "0.1.0") + t.Chdir(root) + resetRunnablesFlags(t) + require.NoError(t, os.Remove(filepath.Join(moduleDir, modulecontract.FileName))) + before := snapshotTree(t, moduleDir) + + require.Error(t, RunnablesCmd.RunE(RunnablesCmd, []string{"documents"}), + "an annotated method with no audience cannot publish without a principal") + require.Equal(t, before, snapshotTree(t, moduleDir), "and writes nothing") +} diff --git a/docs/commands.md b/docs/commands.md index 543eccbc3..15350f9e2 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -152,6 +152,41 @@ run selected (`--env`), the excluded root's file included. A writer handed no environment refuses by name rather than writing a document no reader could find. +**What `--output-env` can represent, and what it refuses.** The file is raw +`KEY=value` lines, and the SDK reader that imports it +(`LoadRuntimeEnvironmentFile`) scans lines, strips one trailing CR, cuts at the +first `=`, and refuses the **whole** artifact if any name is not `CODEFLY__` +plus at least one character of `A`–`Z`, `0`–`9` or `_`. So before its first +write the run holds the complete output batch — own, workspace, dependency and +runtime configuration, `--set` and derived overrides, identity, fixture and +endpoint records alike — to exactly that grammar, and refuses any record it +cannot represent: + +- a name outside the reader's grammar (no `CODEFLY__` prefix, no suffix, + lowercase, hyphen, whitespace, non-ASCII, or carrying `=`, CR, LF or NUL); +- a **value** containing LF, CR or NUL, whichever record it belongs to. A value + carrying a line break is the reason the rule covers every record and not just + the interesting ones: the reader would read its tail as a further `KEY=value` + line, so an unrelated note could substitute a value the run never offered. + +Empty values, and values holding one or many `=`, remain representable — the +reader splits on the first separator only. A refused batch leaves an absent +file absent and an existing one byte-identical; a malformed record already in +an existing file is refused rather than rewritten under this run's name. +Direct process delivery is unaffected and still carries a value's exact bytes, +line breaks included: this is a limit of the raw file format, not of +configuration. + +Two consequences worth stating plainly. An override whose key is not a +`CODEFLY__` carrier (`--set FOO=bar`) now refuses the export rather than +writing a file the reader would reject wholesale. And a **structured +configuration document** cannot be exported this way at all: its carrier is a +prefix plus a lowercase hex SHA256, which that grammar does not admit, so the +reader has never been able to load such a file. The CLI now says so before +writing instead of reporting success. Making it exportable needs an uppercase +document carrier in core, or a reader in the SDK that accepts this one; neither +is the CLI's to decide. + **The delivery host's federation surface.** In a host environment the native run hands the service `host.delivery` names, when it declares the `federation` workspace configuration, the three keys that turn its delivery surface on, diff --git a/docs/configuration-contract.md b/docs/configuration-contract.md index d0bb05fc9..d37a17dd1 100644 --- a/docs/configuration-contract.md +++ b/docs/configuration-contract.md @@ -115,6 +115,71 @@ identity on a managed service uses service's own identity uses `["service-identity"]`. Proxy containers, image choices and loopback routing are not part of this contract. +## Public module artifacts + +A workspace configuration value may be **bound to a file in a composed +module** instead of being written out as a literal, so a public artifact a +module already ships — an installation document, for example — reaches services +as configuration without being copied into the composition and kept in step by +hand. + +The binding is declared in the selected profile, in +`configurations//module-artifacts.codefly.json`, and names only a +destination and a source: + +```json +{ + "schemaVersion": 1, + "bindings": [ + {"configuration": "docs", "key": "README", + "source": {"module": "wiki", "path": "docs/install.md"}} + ] +} +``` + +**Acquired once per ordinary load, and nothing is frozen across loads.** Every +ordinary configuration load — a run's delivery, a `codefly doctor` check — +acquires the declared file afresh and carries its exact bytes, with the SHA256 +and byte count core computes *from those bytes*, on the value that load +delivers. A standalone check therefore describes what *it* observed and proves +nothing about a later delivery; an already running service keeps the +configuration it was delivered until its ordinary lifecycle replaces it. A run +whose planning read saw one version and whose delivery load saw another +delivers the second, and never presents the first as its evidence. + +**What the CLI refuses.** The declared path is opened under the selected +module's directory with confined, non-blocking, regular-file access, so an +absolute path, a `..` element, a symlink leaving the module, a FIFO, a +directory and a missing file all refuse; so does a file that changed while it +was being read. A composed module this machine has not materialized refuses +rather than being fetched behind the read, and a path the CLI materialized +needs a resolution receipt that answers the request the workspace makes now. An +explicit `path:`, an overlay path or a worktree legitimately has no receipt: +its bytes are local development evidence, not release authenticity. Core then +bounds the bytes at the inline carrier threshold (32 KiB, inclusive), requires +valid UTF-8 with no NUL, and refuses text that would need interpolation. + +**No value can quietly take a bound destination.** A literal, secret, template +or structured document offered for the same destination is a conflict rather +than a silent winner, so moving an existing literal onto a binding is an +explicit source edit: remove the literal and add the binding. At delivery the +same rule holds against *additional* offers — a configuration the agent returns +from Init, a dependency's configuration at Start, a `--set` or derived runtime +override — including one that offers the identical bytes. Equality buys no +replacement right, and an empty override map does not skip the check. Removing +a binding restores ordinary ownership on the next load; there is no generated +state to clean up. + +**An explicit binding is the author's assertion that these bytes are public.** +Confinement and a digest do not establish publicness, and no scanner or +filename rule can: a binding declaring private or mixed-secret content as +public exposes both the content and a testable digest. That judgement belongs +to whoever reviews the composition. + +Such a value is delivered inline to the process. It cannot be exported through +`--output-env` when its bytes contain a line break, which that raw format +cannot represent (see `docs/commands.md`). + ## Module-owned keys A few keys on an environment belong to a **module agent**, not to the CLI. The diff --git a/docs/runnable.md b/docs/runnable.md index 578168245..731d7d841 100644 --- a/docs/runnable.md +++ b/docs/runnable.md @@ -43,13 +43,39 @@ owns the option, the descriptor-to-bounded-schema projection and published contracts, the layout on disk and the drift gate. Nothing about the contract is authored twice — it is the projection of the method's own messages. -The selected owner's `module.contract.codefly.yaml` supplies the audience through -Core's strict module-contract reader. Its principal is independent of the -producer checkout's module name; generation keeps the package's module identity -unchanged. Every generated destination must be published by that contract, and -an explicit operation audience must match its principal. Missing, unreadable or -contradictory publication refuses generation before output changes. A module -with no operations still writes an empty index. +Where an operation's audience comes from decides what the module must publish. + +A method that **annotates its audience explicitly** (Core sets +`OperationSpec.Declared` from the method option) carries the author's own +audience. That may be a host surface the module reaches without being installed +as one — a source-operation audience, say — so Core does not require it to equal +any module principal, and neither does generation: such an operation needs no +`module.contract.codefly.yaml` at all. Its served target, scopes and lookup are +still checked as always, so an unserved method or a missing paired Lookup +refuses. + +A method that **carries no operation option** takes Core's default from +`ServiceOwner.Audience`, which generation fills from the selected owner's +`module.contract.codefly.yaml` through Core's strict module-contract reader. +There the contract is required: with no publication there is no principal to +derive from, and generation refuses rather than emit an empty audience. + +The distinction is the option's presence, not an audience's: `Declared` marks +that the method carries the option. Core validates the audience inside it +before setting `Declared`, and generation refuses an annotated method whose +audience is empty rather than treating it as either case — it has no principal +to check against and nothing of its own to carry. + +When that contract **does exist**, the module's publication applies to every +operation it serves, explicit ones included: each generated destination must be +published by it, and an audience differing from its principal is a conflict in +the module's own declarations rather than host permission. Its principal is +independent of the producer checkout's module name; generation keeps the +package's module identity unchanged. + +An unreadable, malformed or escaped contract is never read as absence — a module +that authored one and got it wrong is refused by name. Every refusal happens +before output changes. A module with no operations still writes an empty index. Derivation is endpoint-specific: `catalog.codefly.json`'s `endpoints[].services[].procedures` declares the served surface; descriptor bytes diff --git a/go.mod b/go.mod index 62420b369..0a8df19b6 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,7 @@ require ( github.com/codefly-dev/authority/libraries/delivery/go v0.0.0-20261026023425-000d44495d93 github.com/codefly-dev/authority/libraries/fixture-composition/go v0.0.0-20261027171336-5178f01696e3 github.com/codefly-dev/authority/libraries/hop/go v0.0.0-20261026175541-3afad7224855 - github.com/codefly-dev/core v0.19.10 + github.com/codefly-dev/core v0.19.11-0.20261028153419-de19eba80550 github.com/codefly-dev/golor v0.1.3 github.com/codefly-dev/llm v0.1.7 github.com/codefly-dev/sdk-go/workcontext v0.1.1-0.20261018052022-70dc7a546458 diff --git a/go.sum b/go.sum index f118e5da5..31175fa7a 100644 --- a/go.sum +++ b/go.sum @@ -119,6 +119,8 @@ github.com/codefly-dev/authority/libraries/resources/go v0.0.0-20261026023425-00 github.com/codefly-dev/authority/libraries/resources/go v0.0.0-20261026023425-000d44495d93/go.mod h1:1rmjrVyr0h7zOuwOHS9i+BoeS9dr3XNvtupLZFwY6As= github.com/codefly-dev/core v0.19.10 h1:rTeh1vph0xGtpiCCt2Dl1AH2Rz9hzIYt1YGcbv6bLYE= github.com/codefly-dev/core v0.19.10/go.mod h1:aTJY3Rw0OSVkbWRThgDzSkqieoF9mW4MiPRXM8Vzxww= +github.com/codefly-dev/core v0.19.11-0.20261028153419-de19eba80550 h1:IBur1pFM7LelM+9ecMgl2/Mxgd+04rgesSVFm3R1k0E= +github.com/codefly-dev/core v0.19.11-0.20261028153419-de19eba80550/go.mod h1:aTJY3Rw0OSVkbWRThgDzSkqieoF9mW4MiPRXM8Vzxww= github.com/codefly-dev/golor v0.1.3 h1:xmo+ceyJFRYZdvpWE2fNd0jeaadp/Ibm1BnganiGKOc= github.com/codefly-dev/golor v0.1.3/go.mod h1:sl/u/K1l7J0Pr3xyVZp8fOJYQItKKst1No9JqgzLLoY= github.com/codefly-dev/gortk v0.2.0 h1:7bOlS5valYz2zil+fZctQNcPCYBcPj86abcw9N8h1hQ= diff --git a/pkg/cli/output_sink.go b/pkg/cli/output_sink.go index 4d8080e69..cdfb3e654 100644 --- a/pkg/cli/output_sink.go +++ b/pkg/cli/output_sink.go @@ -1,29 +1,34 @@ package cli import ( - "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/wool" ) -// flowOutputSink adapts the package-level CLI narration functions to +// FlowOutputSink adapts the package-level CLI narration functions to // orchestration.OutputSink, so cobra commands can keep their existing // stdout narration now that pkg/orchestration no longer imports pkg/cli // directly. -type flowOutputSink struct{} +// +// It satisfies that interface structurally and does not name it, which is +// what keeps pkg/cli BELOW pkg/orchestration in the import graph: naming it +// put pkg/cli above orchestration, and since pkg/composition narrates through +// pkg/cli, orchestration could then not read a composed module's tree +// (module_artifact.go) without a cycle. +type FlowOutputSink struct{} // NewOutputSink returns the pkg/cli-backed orchestration.OutputSink. -func NewOutputSink() orchestration.OutputSink { - return flowOutputSink{} +func NewOutputSink() FlowOutputSink { + return FlowOutputSink{} } -func (flowOutputSink) Process(log *wool.Log) { GetLogger().Process(log) } +func (FlowOutputSink) Process(log *wool.Log) { GetLogger().Process(log) } -func (flowOutputSink) ProcessWithSource(source *wool.Identifier, log *wool.Log) { +func (FlowOutputSink) ProcessWithSource(source *wool.Identifier, log *wool.Log) { GetLogger().ProcessWithSource(source, log) } -func (flowOutputSink) Info(format string, args ...any) { Info(format, args...) } +func (FlowOutputSink) Info(format string, args ...any) { Info(format, args...) } -func (flowOutputSink) Error(format string, args ...any) { Error(format, args...) } +func (FlowOutputSink) Error(format string, args ...any) { Error(format, args...) } -func (flowOutputSink) RegisterLoggingResource(unique string) { RegisterLoggingResource(unique) } +func (FlowOutputSink) RegisterLoggingResource(unique string) { RegisterLoggingResource(unique) } diff --git a/pkg/composition/fixtures.go b/pkg/composition/fixtures.go index 51ba82717..64e96fd24 100644 --- a/pkg/composition/fixtures.go +++ b/pkg/composition/fixtures.go @@ -12,7 +12,6 @@ import ( "github.com/codefly-dev/cli/pkg/cli" "github.com/codefly-dev/cli/pkg/environments" - "github.com/codefly-dev/cli/pkg/orchestration" corecomposition "github.com/codefly-dev/core/composition" "github.com/codefly-dev/core/resources" ) @@ -197,7 +196,7 @@ func ValidateFixtureSelection(ctx context.Context, workspace *resources.Workspac // hand at each call site, so only the one that happened to be covered was ever // exercised by a test. func ResolveFixtureSelection(ctx context.Context, workspace *resources.Workspace, env *environments.Environment, override string) (string, error) { - selected := orchestration.SelectedFixture(env, override) + selected := environments.SelectedFixture(env, override) if err := ValidateFixtureSelection(ctx, workspace, selected); err != nil { return "", err } diff --git a/pkg/composition/module_selection.go b/pkg/composition/module_selection.go new file mode 100644 index 000000000..6fe0822b8 --- /dev/null +++ b/pkg/composition/module_selection.go @@ -0,0 +1,94 @@ +package composition + +import ( + "context" + "fmt" + "path/filepath" + + "github.com/codefly-dev/core/resources" +) + +// SelectedModuleDir is the directory a composed module resolves to right now, +// judged under the owner rules the materializer itself uses, and nothing else. +// +// It never pulls, materializes or reloads: a caller that reads bytes out of a +// composed module reads what this invocation already selected, so a reference +// the workspace cannot load as a local checkout is refused rather than fetched +// behind the read (ResolveComposedModuleDir is the materializing half, for +// callers whose job is to make the tree exist). +// +// Provenance decides how hard that refusal is. A path the CLI materialized — +// pinnedManaged, the same predicate the materializer gates its own writes on — +// must have a receipt that answers the request this workspace makes now, or +// the bytes under it are from some other resolution and the selection is +// stale. An explicit `path:`, an overlay path or a worktree legitimately has +// no receipt at all: it is the developer's own checkout, selected by them, and +// its bytes are local development evidence rather than release authenticity. +// Returning verified=false for those is the whole point — a caller that needs +// to say where bytes came from must not call a worktree a release. +func SelectedModuleDir(ctx context.Context, workspace *resources.Workspace, module string) (dir string, verified bool, err error) { + var ref *resources.ModuleReference + for _, candidate := range workspace.Modules { + if candidate.Name == module { + ref = candidate + break + } + } + if ref == nil { + return "", false, fmt.Errorf("module %s is not composed in workspace %s", module, workspace.Name) + } + state, err := loadMaterializationState(ctx, workspace) + if err != nil { + return "", false, err + } + var directive *resources.ModuleResolveDirective + if state.overlay != nil { + directive = state.overlay.Resolve[module] + } + receipt := state.receipts[module] + mode := ResolutionModeFor(ref, directive, receipt, state.declared[module]) + resolution, err := workspace.ResolveModule(ctx, ref) + if err != nil { + return "", false, err + } + if resolution.Kind == resources.ResolutionPinned { + return "", false, fmt.Errorf("module %s is not materialized on this machine; resolve the composition before reading its tree", module) + } + if pinnedManaged(ref, directive, receipt.ResolvedPath(), state.roots.owned...) { + if !moduleReceiptAnswers(ref, directive, receipt, mode) { + return "", false, fmt.Errorf("module %s materialization is unresolved or stale; resolve the composition before reading its tree", module) + } + // Two sources had to agree to get here, and they are read at different + // moments: the overlay and the receipts come off disk NOW, while + // workspace.ResolveModule answers from the overlay snapshot the + // workspace was LOADED with. A selection changed in between — a + // re-materialization, an edited overlay — makes the receipt describe a + // path the resolution no longer names, and the bytes would then be read + // from one selection while attributed to another's receipt. Require the + // resolved directory to be the receipt's path, or under it where the + // reference names a module subpath, and refuse the divergence rather + // than pick a side. + if !sameSelectedDir(resolution.Dir, receipt.ResolvedPath()) { + return "", false, fmt.Errorf("module %s resolves to a different directory than its materialization receipt records; resolve the composition again before reading its tree", module) + } + return resolution.Dir, !mode.Unverified(), nil + } + return resolution.Dir, false, nil +} + +// sameSelectedDir compares the resolved directory with the receipt's, and +// requires them to be the SAME directory — not one under the other. +// +// The materializer records the module directory itself, with ref.Module's +// subpath already joined on (updateReceipt stores the resolved module dir, not +// the checkout root), so the receipt and the resolution are two spellings of +// one directory and nothing legitimately nests. A prefix test would pass a +// stale selection that happens to sit beneath the current one — exactly the +// case the reviewer's A-nested-under-B repro covers — so only normalized +// equality can tell "the same selection" from "a different one inside it". +func sameSelectedDir(dir, selected string) bool { + if dir == "" || selected == "" { + return false + } + return filepath.Clean(dir) == filepath.Clean(selected) +} diff --git a/pkg/conformance/matrix.json b/pkg/conformance/matrix.json index 2b3cd329f..b3e448688 100644 --- a/pkg/conformance/matrix.json +++ b/pkg/conformance/matrix.json @@ -1,7 +1,7 @@ { "schema_version": 1, "cli": "source", - "core": "v0.19.10", + "core": "v0.19.11", "rows": [ { "id": "linux-amd64-source", diff --git a/pkg/environments/environment.go b/pkg/environments/environment.go index b9c81b0ee..18fefcc4f 100644 --- a/pkg/environments/environment.go +++ b/pkg/environments/environment.go @@ -1932,3 +1932,23 @@ func (s *EnvironmentServiceSecrets) validateGenerators() error { } return nil } + +// SelectedFixture resolves the fixture a workspace-bound flow runs with. An +// environment declares the fixture its runtime should use, so an ordinary +// `codefly run service` / `codefly test service` needs no --fixture at all. +// An explicit override always wins, and an environment that deliberately +// omits a fixture — because it loads a real provider — resolves to none. +// +// It lives here, beside the declaration, rather than in the orchestration +// package that is its main caller, because the composition package resolves a +// fixture selection too — and orchestration now reads composed module trees +// (module_artifact.go), so the two cannot both depend on each other. +func SelectedFixture(environment *Environment, override string) string { + if override != "" { + return override + } + if environment == nil { + return "" + } + return environment.Fixture +} diff --git a/pkg/environments/product_secrets.go b/pkg/environments/product_secrets.go index 84c0fdee3..e401e7f21 100644 --- a/pkg/environments/product_secrets.go +++ b/pkg/environments/product_secrets.go @@ -64,7 +64,12 @@ func (policy *ProductSecretPolicy) Validate() error { // secret, and keys that group declares; a may-be-empty entry names a secret key // some declared group produces. Anything else is refused by name: it would // generate, or excuse, a value no rendered service reads. -func ValidateSecretPolicy(ctx context.Context, workspace *resources.Workspace, env *Environment, policy *ProductSecretPolicy) error { +// The resolver is a parameter rather than something this package builds: a +// module artifact's bytes are acquired from a composed module under the +// composition's own provenance rules, which live above pkg/environments. A +// caller that omits it reads a composition with a bound artifact as a refusal +// from Core — never as an absent value. +func ValidateSecretPolicy(ctx context.Context, workspace *resources.Workspace, env *Environment, policy *ProductSecretPolicy, resolvers ...configurations.ModuleArtifactResolver) error { if policy.Empty() { return nil } @@ -81,6 +86,9 @@ func ValidateSecretPolicy(ctx context.Context, workspace *resources.Workspace, e composed[identity.Unique()] = true } reader, err := configurations.NewConfigurationLocalReader(ctx, workspace) + for _, resolve := range resolvers { + reader = reader.WithModuleArtifactResolver(resolve) + } if err != nil { return err } diff --git a/pkg/gitops/authority.go b/pkg/gitops/authority.go index ce6594080..a7f682831 100644 --- a/pkg/gitops/authority.go +++ b/pkg/gitops/authority.go @@ -12,6 +12,7 @@ import ( compositionv1 "github.com/codefly-dev/authority/libraries/authority-client/go/gen/host/composition/v1" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/configurations" modulecontract "github.com/codefly-dev/core/contracts/module" "github.com/codefly-dev/core/resources" @@ -88,7 +89,7 @@ func moduleAuthority( if request.contract.Principal != module.Name { return nil, fmt.Errorf("module %s publishes a contract whose principal is %q; the principal every credential of a module is issued to is the module's own name, so the contract declares principal: %s", module.Name, request.contract.Principal, module.Name) } - provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime()) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime(), orchestration.ModuleArtifactResolverFor(workspace)) if err != nil { return nil, fmt.Errorf("read the workspace configurations the contract of %s resolves against: %w", module.Name, err) } diff --git a/pkg/gitops/configuration_groups.go b/pkg/gitops/configuration_groups.go index 2073cb57d..78e364d17 100644 --- a/pkg/gitops/configuration_groups.go +++ b/pkg/gitops/configuration_groups.go @@ -14,6 +14,7 @@ import ( "strings" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/configurations" modulecontract "github.com/codefly-dev/core/contracts/module" "github.com/codefly-dev/core/resources" @@ -79,7 +80,7 @@ func workspaceConfigurationDigests(ctx context.Context, workspace *resources.Wor // them now; a group the environment does not provide is absent from the // result. func digestProvidedGroups(ctx context.Context, workspace *resources.Workspace, env *environments.Environment, consumed map[string]struct{}) (map[string]string, error) { - provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime()) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime(), orchestration.ModuleArtifactResolverFor(workspace)) if err != nil { return nil, fmt.Errorf("read the workspace configurations this render bakes in: %w", err) } diff --git a/pkg/gitops/module_authority.go b/pkg/gitops/module_authority.go index eadea06c0..bad79d999 100644 --- a/pkg/gitops/module_authority.go +++ b/pkg/gitops/module_authority.go @@ -7,6 +7,7 @@ import ( "slices" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/configurations" basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" "github.com/codefly-dev/core/resources" @@ -17,7 +18,7 @@ import ( // Read through Core so composed modules, imports and profile selection follow // the same rules as every other workspace configuration. func refuseAuthoredRenderConfigurations(ctx context.Context, workspace *resources.Workspace, env *resources.Environment) error { - provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env, orchestration.ModuleArtifactResolverFor(workspace)) if err != nil { return err } diff --git a/pkg/gitops/runnable_grants.go b/pkg/gitops/runnable_grants.go index 71527c84d..6c3a35166 100644 --- a/pkg/gitops/runnable_grants.go +++ b/pkg/gitops/runnable_grants.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" runnablespkg "github.com/codefly-dev/cli/pkg/runnables" "github.com/codefly-dev/core/configurations" modulecontract "github.com/codefly-dev/core/contracts/module" @@ -239,7 +240,7 @@ func WithPreparedBindingGrants(ctx context.Context, workspace *resources.Workspa if len(installers) == 0 { return principals, nil } - provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime()) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime(), orchestration.ModuleArtifactResolverFor(workspace)) if err != nil { return nil, fmt.Errorf("read the prepared bindings the envelope lists: %w", err) } diff --git a/pkg/orchestration/disposable_engine_test.go b/pkg/orchestration/disposable_engine_test.go index 95c667db1..c895a3f32 100644 --- a/pkg/orchestration/disposable_engine_test.go +++ b/pkg/orchestration/disposable_engine_test.go @@ -377,10 +377,18 @@ func disposableBuiltRef(call disposableEngineCall) string { func disposableOrdinaryRuntimeRef(t *testing.T, b *Builder) string { t.Helper() ordinary := *b - world := *b.world - world.disposableImageScope = "" - world.RebuildImages = true // identity/name only; do not query a cache for reuse - ordinary.world = &world + // By pointer, never a copy: World carries a sync.Once (the memoized + // workspace producer lookup), so `world := *b.world` duplicates a lock — + // vet's copylocks, and a real bug rather than noise, because the copy's + // Once inherits the original's fired state. The two fields the derivation + // needs are overridden on the scoped builder's own world and restored + // before returning, which is sound here because this helper is called from + // a sequential test and executes no Build and no cache write. + scope, rebuild := b.world.disposableImageScope, b.world.RebuildImages + b.world.disposableImageScope = "" + b.world.RebuildImages = true // identity/name only; do not query a cache for reuse + defer func() { b.world.disposableImageScope, b.world.RebuildImages = scope, rebuild }() + ordinary.world = b.world ordinary.localRuntime = true plan := b.planned.response.GetResult().GetDockerBuildPlan() recipe, err := ordinary.recipeAsBuilt(plan.Recipes[0], false) diff --git a/pkg/orchestration/environment.go b/pkg/orchestration/environment.go index fc888a649..cad6da7bd 100644 --- a/pkg/orchestration/environment.go +++ b/pkg/orchestration/environment.go @@ -22,17 +22,9 @@ func SelectEnvironment(workspace *resources.Workspace, name string) (*environmen return environments.Select(workspace, name) } -// SelectedFixture resolves the fixture a workspace-bound flow runs with. An -// environment declares the fixture its runtime should use, so an ordinary -// `codefly run service` / `codefly test service` needs no --fixture at all. -// An explicit override always wins, and an environment that deliberately -// omits a fixture — because it loads a real provider — resolves to none. +// SelectedFixture resolves the fixture a workspace-bound flow runs with, +// where the environment declares it (environments.SelectedFixture, which the +// composition package shares). func SelectedFixture(environment *environments.Environment, override string) string { - if override != "" { - return override - } - if environment == nil { - return "" - } - return environment.Fixture + return environments.SelectedFixture(environment, override) } diff --git a/pkg/orchestration/environment_remote_manager.go b/pkg/orchestration/environment_remote_manager.go index 95e529597..ad1f6032d 100644 --- a/pkg/orchestration/environment_remote_manager.go +++ b/pkg/orchestration/environment_remote_manager.go @@ -21,6 +21,10 @@ func NewEnvironmentRemoteManager(ctx context.Context, workspace *resources.Works if err != nil { return nil, err } + // The same acquisition every other delivery and check load supplies: a + // bound module artifact reaching this manager must be resolved from the + // selected module, not refused as unresolvable (module_artifact.go). + localReader.WithModuleArtifactResolver(ModuleArtifactResolverFor(workspace)) manager.WithLoader(localReader) return remotenetwork.NewRemoteManager(ctx, manager) } diff --git a/pkg/orchestration/excluded_root_output_env.go b/pkg/orchestration/excluded_root_output_env.go index d6777ed07..5828d09fd 100644 --- a/pkg/orchestration/excluded_root_output_env.go +++ b/pkg/orchestration/excluded_root_output_env.go @@ -73,19 +73,6 @@ func (flow *Flow) exportExcludedOriginEnvironment(ctx context.Context) error { if err != nil { return fmt.Errorf("name the run environment for the excluded root's output environment: %w", err) } - if err := AppendServiceProcessConfigurationsToFile( - ctx, - flow.outputEnvPath, - environment, - runtimeContext, - serviceConfiguration, - workspaceConfigurations, - dependencyConfigurations, - nil, - ); err != nil { - return fmt.Errorf("write excluded root configurations: %w", err) - } - protoIdentity := &basev0.ServiceIdentity{ Workspace: identity.Workspace, Module: identity.Module, @@ -102,16 +89,30 @@ func (flow *Flow) exportExcludedOriginEnvironment(ctx context.Context) error { if flow.world.Env != nil && flow.world.Env.NamingScope != "" { overrides[resources.NamingScopePrefix] = flow.world.Env.NamingScope } - if err := AppendRuntimeEnvironmentToFile( - ctx, - flow.outputEnvPath, - protoIdentity, - runtimeContext, - flow.fixture, - overrides, - endpointMappings, - ); err != nil { - return fmt.Errorf("write excluded root runtime environment: %w", err) + // The selected payload is checked against itself and against this export's + // override map — duplicates inside it are still collisions — and the + // complete record batch is converted, before the first append. No agent + // Init runs for an excluded root, so there is no returned contribution and + // no retained delivery to pretend there is one. + selected := deliveryBaseline(serviceConfiguration, workspaceConfigurations, dependencyConfigurations) + if deliveryErr := resources.ValidateModuleArtifactDelivery(selected, nil, overrides); deliveryErr != nil { + return fmt.Errorf("export excluded root environment: %w", deliveryErr) + } + configurationVariables, err := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, + serviceConfiguration, workspaceConfigurations, dependencyConfigurations, nil) + if err != nil { + return fmt.Errorf("convert excluded root configurations: %w", err) + } + runtimeVariables, err := runtimeEnvironmentVariables(ctx, protoIdentity, runtimeContext, flow.fixture, overrides, endpointMappings) + if err != nil { + return fmt.Errorf("convert excluded root runtime environment: %w", err) + } + records, err := outputRecords(append(configurationVariables, runtimeVariables...)) + if err != nil { + return fmt.Errorf("export excluded root environment: %w", err) + } + if err := writeOutputRecords(ctx, flow.outputEnvPath, records); err != nil { + return fmt.Errorf("write excluded root environment: %w", err) } return nil } diff --git a/pkg/orchestration/hop_configurations.go b/pkg/orchestration/hop_configurations.go index 02806d742..14270be0b 100644 --- a/pkg/orchestration/hop_configurations.go +++ b/pkg/orchestration/hop_configurations.go @@ -43,6 +43,12 @@ func NewWorkspaceConfigurationReader(ctx context.Context, workspace *resources.W if err != nil { return nil, err } + // Bound here, so run delivery and every check that asks what this + // invocation resolves acquire module artifacts the same way: a bound + // destination core cannot resolve refuses rather than falling back to a + // literal, and a standalone check observes its own acquisition rather than + // inheriting one (module_artifact.go). + reader.WithModuleArtifactResolver(ModuleArtifactResolverFor(workspace)) return &WorkspaceConfigurationReader{ConfigurationInformationLocalReader: reader, workspace: workspace}, nil } @@ -77,14 +83,34 @@ func (r *WorkspaceConfigurationReader) Load(ctx context.Context, env *resources. if fixture != nil { // Replace any module default with the complete merge. Core may have // resolved a single fragment (or identical fragments) without ambiguity. + // + // What is replaced must not be something Core was asked to deliver. A + // module artifact bound to this group has already been acquired, had + // its bytes copied and its identity computed, and is carried on the + // value Core resolved — deleting that info here would substitute this + // generated document for the artifact the composition declared, with + // nothing said. The whole-group replacement is exactly the lossy + // precedence the delivery contract requires to be checked BEFORE the + // offer is discarded, so it refuses instead. for i, config := range r.combined { if config.GetOrigin() != resources.ConfigurationWorkspace { continue } replacement := proto.CloneOf(config) + var bound bool replacement.Infos = slices.DeleteFunc(replacement.Infos, func(info *basev0.ConfigurationInformation) bool { - return info.GetName() == fixturefragments.Group + if info.GetName() != fixturefragments.Group { + return false + } + bound = bound || slices.ContainsFunc(info.GetConfigurationValues(), func(value *basev0.ConfigurationValue) bool { + return value.GetResolvedModuleArtifact() != nil || value.GetModuleArtifact() != nil + }) + return true }) + if bound { + return fmt.Errorf("%s is bound to a module artifact and is also the merged fixture composition: the generated document would replace the declared artifact, so remove the binding or the fragments (%w)", + fixturefragments.Group, configurations.ErrConfigurationConflict) + } r.combined[i] = replacement } r.combined = append(r.combined, fixture) diff --git a/pkg/orchestration/local_runtime_image.go b/pkg/orchestration/local_runtime_image.go index b3d1d4e93..cd5fcefd6 100644 --- a/pkg/orchestration/local_runtime_image.go +++ b/pkg/orchestration/local_runtime_image.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "path/filepath" + "slices" "strings" "github.com/distribution/reference" @@ -39,6 +40,29 @@ func (runner *Runner) localRuntimeImage(ctx context.Context) (string, error) { if advertised, supported := ValidationOperationSupport(runner.instance.Info, ValidationArtifactBuild); advertised && !supported { return "", nil } + // Whether this run relies on the handoff is decided by the agent + // advertising the optional capability — not by whether its builder happens + // to emit exactly one recipe. + // + // A single recipe does not make a runtime image. A database agent emits one + // for its migration or bootstrap image, and reading the runtime role off + // that sole recipe both built the wrong image and then failed the run for a + // capability the agent never claimed. An agent that does not advertise + // keeps its own image selection, exactly as a stock-image one does, and its + // build stays out of this path entirely. + // + // The test is deliberately the capability list and not contract.Check: + // Check reports a protocol-version mismatch with the same ErrIncompatible + // as a missing capability, so treating any error as "not advertised" would + // silently accept an agent this host cannot speak to. Absence of an + // optional capability is the only thing waived here; the strict guard in + // buildLocalRuntimeImage still runs the full Check once the handoff is + // selected, so an advertising agent with an incompatible protocol is + // refused rather than skipped. The disposable image policy above runs + // before this skip and is never waived by it. + if !slices.Contains(runner.instance.Info.GetContract().GetCapabilities(), contract.RuntimeInitImage) { + return "", nil + } if runner.instance.Builder == nil { if err := runner.instance.LoadBuilder(ctx); err != nil { return "", err @@ -118,6 +142,10 @@ func (b *Builder) buildLocalRuntimeImage(ctx context.Context) (string, error) { if len(plan.GetRecipes()) != 1 { return "", fmt.Errorf("local runtime for %s requires exactly one image recipe, got %d", b.instance.Unique(), len(plan.GetRecipes())) } + // The strict guard, retained: localRuntimeImage only waives the ABSENCE of + // the optional capability, so the full contract check still runs here for + // an agent that did advertise it — a protocol-version mismatch, or any + // other incompatibility, refuses the handoff rather than being skipped. if err := b.instance.RequireAgentCapabilities(contract.RuntimeInitImage); err != nil { return "", fmt.Errorf("cannot supply a local runtime image to %s: %w", b.instance.Unique(), err) } diff --git a/pkg/orchestration/local_runtime_image_test.go b/pkg/orchestration/local_runtime_image_test.go index aee7410c5..b75a17e8f 100644 --- a/pkg/orchestration/local_runtime_image_test.go +++ b/pkg/orchestration/local_runtime_image_test.go @@ -7,6 +7,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -47,17 +48,40 @@ type runtimeImageBuilderPeer struct { imageCachePeer stock bool malformed bool + // recipeName renames the emitted recipe. core's docker.proto makes name + // the recipe's logical identity ("such as app or migration"), so a sole + // recipe named "bootstrap" is a migration runner, not a runtime image. + recipeName string + // loads/inits/builds count the builder RPCs the CLI actually made. An + // agent that does not advertise the handoff must reach NONE of them: a + // zero docker-build count alone would still pass while the CLI loaded, + // initialized and planned a builder whose result it will never use. + loads atomic.Int32 + inits atomic.Int32 + builds atomic.Int32 } -func (*runtimeImageBuilderPeer) Load(context.Context, *builderv0.LoadRequest) (*builderv0.LoadResponse, error) { +func (peer *runtimeImageBuilderPeer) Load(context.Context, *builderv0.LoadRequest) (*builderv0.LoadResponse, error) { + peer.loads.Add(1) return &builderv0.LoadResponse{State: &builderv0.LoadStatus{State: builderv0.LoadStatus_READY}}, nil } -func (*runtimeImageBuilderPeer) Init(context.Context, *builderv0.InitRequest) (*builderv0.InitResponse, error) { +func (peer *runtimeImageBuilderPeer) Init(context.Context, *builderv0.InitRequest) (*builderv0.InitResponse, error) { + peer.inits.Add(1) return &builderv0.InitResponse{State: &builderv0.InitStatus{State: builderv0.InitStatus_SUCCESS}}, nil } +// requireNoBuilderTraffic is the zero-call assertion: no Load, no Init, no +// Build (the plan request) reached the agent's builder at all. +func (peer *runtimeImageBuilderPeer) requireNoBuilderTraffic(t *testing.T) { + t.Helper() + require.Zero(t, peer.loads.Load(), "the builder must not be loaded for an agent that will not use the handoff") + require.Zero(t, peer.inits.Load(), "nor initialized") + require.Zero(t, peer.builds.Load(), "nor asked for a build plan") +} + func (peer *runtimeImageBuilderPeer) Build(ctx context.Context, req *builderv0.BuildRequest) (*builderv0.BuildResponse, error) { + peer.builds.Add(1) if peer.stock { return &builderv0.BuildResponse{ State: &builderv0.BuildStatus{State: builderv0.BuildStatus_SUCCESS}, @@ -65,6 +89,11 @@ func (peer *runtimeImageBuilderPeer) Build(ctx context.Context, req *builderv0.B }, nil } resp, err := peer.imageCachePeer.Build(ctx, req) + if err == nil && peer.recipeName != "" { + for _, recipe := range resp.GetResult().GetDockerBuildPlan().GetRecipes() { + recipe.Name = peer.recipeName + } + } if err == nil && peer.malformed { err = os.WriteFile(filepath.Join(req.GetOutputDirectory(), "Dockerfile"), []byte("FROM tampered\n"), 0o644) } @@ -72,9 +101,14 @@ func (peer *runtimeImageBuilderPeer) Build(ctx context.Context, req *builderv0.B } func runtimeImageRunner(t *testing.T, peer *runtimeImageBuilderPeer) (*Runner, *dockerStub) { + t.Helper() + return runtimeImageRunnerWithAgent(t, peer, realAgentInputs) +} + +func runtimeImageRunnerWithAgent(t *testing.T, peer *runtimeImageBuilderPeer, mode string) (*Runner, *dockerStub) { t.Helper() stub := installDockerStub(t, true) - runner, world := gatewayRunner(t, startRealAgent(t, realAgentInputs)) + runner, world := gatewayRunner(t, startRealAgent(t, mode)) root := t.TempDir() world.Workspace.WithDir(root) serviceDir := filepath.Join(root, "modules/web/services/gateway") @@ -145,11 +179,80 @@ esac } } -func TestRunnerRefusesImageHandoffWithoutLiveCapability(t *testing.T) { - runner, stub := runtimeImageRunner(t, &runtimeImageBuilderPeer{}) +// TestRunnerLeavesTheImageToANonAdvertisingAgent is the Postgres shape, and +// the behaviour that replaces an earlier refusal. +// +// The agent has a builder and its builder emits a recipe — but that recipe is +// not its runtime image (a database's is its migration or bootstrap image), and +// it never advertised runtime-init-image/v1. So the run must leave the image to +// the agent: no build, no handoff, no refusal. Reading the runtime role off the +// sole recipe instead built the wrong image and then failed the run for a +// capability the agent never claimed, which is what blocked local startup. +func TestRunnerLeavesTheImageToANonAdvertisingAgent(t *testing.T) { + peer := &runtimeImageBuilderPeer{} + runner, stub := runtimeImageRunner(t, peer) + runner.instance.Info.Contract = contract.Current() + require.NotContains(t, runner.instance.Info.GetContract().GetCapabilities(), contract.RuntimeInitImage) + + output, err := runner.Init(t.Context()) + require.NoError(t, err, "a non-advertising agent owns its image; the run must not refuse") + require.NotNil(t, output, "and Init must complete normally, not short-circuit") + require.Zero(t, stub.calls(t, "build"), "its recipe must not be built as a runtime image") + require.NotContains(t, exposedValues(t.Context(), t, runner.world), "runtime-image") + // The decision is taken before the builder is touched at all. + peer.requireNoBuilderTraffic(t) +} + +// TestRunnerPropagatesTheAgentsOwnInitFailure keeps the early return from +// becoming a swallow-all. Skipping the handoff must not also skip — or absorb — +// the runtime agent's own Init outcome: a non-advertising agent whose Init +// fails still fails the run, with its own diagnostic. +func TestRunnerPropagatesTheAgentsOwnInitFailure(t *testing.T) { + peer := &runtimeImageBuilderPeer{} + runner, _ := runtimeImageRunnerWithAgent(t, peer, realAgentRefuses) runner.instance.Info.Contract = contract.Current() + + _, err := runner.Init(t.Context()) + require.Error(t, err, "the agent's own Init failure must reach the caller") + require.ErrorContains(t, err, realAgentRefusalMessage) + require.NotContains(t, err.Error(), contract.RuntimeInitImage, + "and it must not be reported as a handoff-capability problem") + peer.requireNoBuilderTraffic(t) +} + +// TestRunnerLeavesTheImageToAnAgentPinningItsOwnRuntime is the third class the +// earlier model had no case for, and the one service-postgres is: a stock or +// pinned runtime image PLUS a build recipe for a DIFFERENT image (its +// migration/bootstrap runner). core's docker.proto says a recipe's name is its +// logical identity and that a service may emit several, so cardinality cannot +// say which recipe — if any — is the runtime image. Here the sole recipe is +// named "bootstrap", and it must not be built or handed to Runtime.Init. +func TestRunnerLeavesTheImageToAnAgentPinningItsOwnRuntime(t *testing.T) { + peer := &runtimeImageBuilderPeer{recipeName: "bootstrap"} + runner, stub := runtimeImageRunner(t, peer) + runner.instance.Info.Contract = contract.Current() + + _, err := runner.Init(t.Context()) + require.NoError(t, err, "a migration recipe is not a declaration of the runtime image") + require.Zero(t, stub.calls(t, "build")) + require.NotContains(t, exposedValues(t.Context(), t, runner.world), "runtime-image") + peer.requireNoBuilderTraffic(t) +} + +// TestRunnerRefusesAnIncompatibleAdvertisingAgent keeps the waiver specific. +// contract.Check reports a protocol-version mismatch with the same +// ErrIncompatible as a missing capability, so an agent that DID advertise the +// handoff and cannot speak this host's protocol must still be refused — not +// quietly treated as one that owns its own image. +func TestRunnerRefusesAnIncompatibleAdvertisingAgent(t *testing.T) { + runner, stub := runtimeImageRunner(t, &runtimeImageBuilderPeer{}) + runner.instance.Info.GetContract().ProtocolVersion = contract.Current().GetProtocolVersion() + 1 + require.Contains(t, runner.instance.Info.GetContract().GetCapabilities(), contract.RuntimeInitImage) + _, err := runner.Init(t.Context()) - require.ErrorContains(t, err, contract.RuntimeInitImage) + require.ErrorIs(t, err, contract.ErrIncompatible, + "an incompatible protocol must refuse, not be waived as a missing optional capability") + require.ErrorContains(t, err, "CLI-agent protocol version") require.Zero(t, stub.calls(t, "build")) } @@ -160,9 +263,15 @@ func TestRunnerRefusesMalformedRuntimeRecipe(t *testing.T) { require.Zero(t, stub.calls(t, "build")) } -func TestRunnerStockImageNeedsNoHandoffCapability(t *testing.T) { +// TestRunnerAdvertisingAgentWithNoRecipeKeepsItsOwnImage is the other half: +// the agent advertises the handoff, so the capability gate lets it through, +// and its builder emits no recipe at all (a stock-image service). That is not +// a refusal either — the agent keeps its own selection. It deliberately +// advertises, so the assertion exercises the no-recipe path rather than +// returning early at the capability gate. +func TestRunnerAdvertisingAgentWithNoRecipeKeepsItsOwnImage(t *testing.T) { runner, stub := runtimeImageRunner(t, &runtimeImageBuilderPeer{stock: true}) - runner.instance.Info.Contract = contract.Current() + require.Contains(t, runner.instance.Info.GetContract().GetCapabilities(), contract.RuntimeInitImage) _, err := runner.Init(t.Context()) require.NoError(t, err) require.NotContains(t, exposedValues(t.Context(), t, runner.world), "runtime-image") diff --git a/pkg/orchestration/module_artifact.go b/pkg/orchestration/module_artifact.go new file mode 100644 index 000000000..0004cd872 --- /dev/null +++ b/pkg/orchestration/module_artifact.go @@ -0,0 +1,134 @@ +package orchestration + +import ( + "context" + "fmt" + "io" + "os" + "syscall" + + "github.com/codefly-dev/cli/pkg/composition" + "github.com/codefly-dev/core/configurations" + basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" + "github.com/codefly-dev/core/resources" + "google.golang.org/protobuf/proto" +) + +// moduleArtifactResolverFor is the acquisition half of core's per-load module +// artifact seam: core owns the declaration, the profile precedence, the +// collision checks and the typed identity it computes from the bytes; the +// selected module, the confined read and the provenance of what was read are +// the CLI's, because only the CLI knows how this invocation resolved that +// module. +// +// Each ordinary Load acquires afresh — core shares one acquisition for the +// repeated uses WITHIN a load and starts clean on the next one — so this +// resolver reads the workspace as the load sees it and never caches across +// loads. A later load may legitimately see changed bytes; nothing here freezes +// them, and nothing rereads the file at emission to prove they were stable. +func ModuleArtifactResolverFor(workspace *resources.Workspace) configurations.ModuleArtifactResolver { + return func(ctx context.Context, ref *basev0.ConfigurationModuleArtifactReference) ([]byte, error) { + return acquireModuleArtifact(ctx, workspace, ref) + } +} + +// acquireModuleArtifact reads the declared path out of the already selected +// module, under that module's directory and nowhere else. +// +// The confinement is os.Root, so a symlink, a `..` element or an absolute path +// cannot leave the selection even if it survived core's reference validation. +// os.Root alone is not enough, though: it still happily opens a FIFO and then +// blocks forever on the read, and it says nothing about what was opened. So the +// open is non-blocking, the mode is checked before a byte is read, and the same +// open file is stat'ed again afterwards — a file replaced or rewritten under +// the read refuses rather than delivering a mixture of two versions. That is a +// same-file observation, not a snapshot of the tree against arbitrary +// concurrent mutation. +func acquireModuleArtifact(ctx context.Context, workspace *resources.Workspace, ref *basev0.ConfigurationModuleArtifactReference) ([]byte, error) { + if workspace == nil { + return nil, fmt.Errorf("module artifacts require a selected workspace") + } + if err := resources.ValidateModuleArtifactReference(ref); err != nil { + return nil, err + } + dir, _, err := composition.SelectedModuleDir(ctx, workspace, ref.Module) + if err != nil { + return nil, err + } + root, err := os.OpenRoot(dir) + if err != nil { + return nil, fmt.Errorf("cannot confine module artifact reads to module %s", ref.Module) + } + defer root.Close() + file, err := root.OpenFile(ref.Path, os.O_RDONLY|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, fmt.Errorf("cannot open module artifact %q in module %s", ref.Path, ref.Module) + } + defer file.Close() + before, err := file.Stat() + if err != nil { + return nil, fmt.Errorf("cannot inspect module artifact %q in module %s", ref.Path, ref.Module) + } + if !before.Mode().IsRegular() { + return nil, fmt.Errorf("module artifact %q in module %s must be a regular file", ref.Path, ref.Module) + } + // One byte over core's inclusive inline bound is read deliberately: core + // refuses the oversize value by its own threshold, and reading no further + // keeps an enormous file from being pulled into memory to be rejected. + content, err := io.ReadAll(io.LimitReader(file, resources.FileCarrierThreshold+1)) + if err != nil { + return nil, fmt.Errorf("cannot read module artifact %q in module %s", ref.Path, ref.Module) + } + after, err := file.Stat() + if err != nil { + return nil, fmt.Errorf("cannot re-inspect module artifact %q in module %s", ref.Path, ref.Module) + } + if !os.SameFile(before, after) || after.Size() != before.Size() || !after.ModTime().Equal(before.ModTime()) || int64(len(content)) != before.Size() { + return nil, fmt.Errorf("module artifact %q in module %s changed while it was being read", ref.Path, ref.Module) + } + // And the SELECTION, re-asked around the read. The same-open-file checks + // above only say the file did not move under us; they say nothing about + // whether this is still the file the composition selects. A + // re-materialization or an edited overlay between the open and here + // re-points the module, and these bytes would then be delivered as the + // selected module's while another directory is what the next load reads. + // Re-resolving is cheap (no pull, no reload) and its refusal is the same + // one as before the read. + reselected, _, err := composition.SelectedModuleDir(ctx, workspace, ref.Module) + if err != nil { + return nil, err + } + if reselected != dir { + return nil, fmt.Errorf("module %s was reselected while artifact %q was being read; resolve the composition again", ref.Module, ref.Path) + } + return content, nil +} + +// deliveryBaseline is a phase's configuration payload as core must see it: +// every offer, in the order it was gathered, with nothing merged, deduplicated +// or reduced to a map first. A duplicate coordinate, an alias and a repeated +// pointer all stay visible, because the offer that would be LOST to precedence +// is exactly the one core has to refuse. +func deliveryBaseline(own *basev0.Configuration, workspace, dependencies []*basev0.Configuration) []*basev0.Configuration { + baseline := make([]*basev0.Configuration, 0, len(workspace)+len(dependencies)+1) + if own != nil { + baseline = append(baseline, own) + } + baseline = append(baseline, workspace...) + return append(baseline, dependencies...) +} + +// detachConfigurations is the retained copy of a delivered payload: typed +// clones, so the generated artifact evidence travels with each value and a +// later load, filter or clone of the original slices cannot quietly rewrite +// what this service was delivered. +func detachConfigurations(confs []*basev0.Configuration) []*basev0.Configuration { + detached := make([]*basev0.Configuration, 0, len(confs)) + for _, conf := range confs { + if conf == nil { + continue + } + detached = append(detached, proto.CloneOf(conf)) + } + return detached +} diff --git a/pkg/orchestration/module_artifact_delivery_test.go b/pkg/orchestration/module_artifact_delivery_test.go new file mode 100644 index 000000000..189e82415 --- /dev/null +++ b/pkg/orchestration/module_artifact_delivery_test.go @@ -0,0 +1,220 @@ +package orchestration + +import ( + "context" + "os" + "path/filepath" + "testing" + + basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" + "github.com/codefly-dev/core/resources" + "github.com/stretchr/testify/require" +) + +// boundWorkspace is a composition whose selected profile binds one public +// module artifact to a workspace destination: the shape Product would author +// for the reviewed Documents installation. The artifact lives in the composed +// module, selected by an explicit committed path. +func boundWorkspace(t *testing.T, body string, extra map[string]string) *resources.Workspace { + t.Helper() + files := map[string]string{ + "workspace.codefly.yaml": `name: artifact-delivery +layout: modules +modules: + - name: wiki + path: modules/wiki +`, + "modules/wiki/module.codefly.yaml": excludedRootExportModule, + "modules/wiki/services/backend/service.codefly.yaml": excludedRootExportService + "workspace-configuration-dependencies:\n - docs\n", + "modules/wiki/docs/install.md": body, + "configurations/local/module-artifacts.codefly.json": `{ + "schemaVersion": 1, + "bindings": [ + {"configuration": "docs", "key": "README", "source": {"module": "wiki", "path": "docs/install.md"}} + ] +}`, + } + for name, content := range extra { + files[name] = content + } + return writeTempWorkspace(t, files) +} + +// loadBound reads the workspace configurations the way a run and a check both +// do — through the CLI's own reader, which is where the resolver is bound. +func loadBound(t *testing.T, workspace *resources.Workspace) []*basev0.Configuration { + t.Helper() + reader, err := NewWorkspaceConfigurationReader(context.Background(), workspace) + require.NoError(t, err) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + require.NoError(t, reader.Load(context.Background(), env.Runtime())) + return reader.Configurations() +} + +// boundValue finds the delivered value for the bound destination. +func boundValue(confs []*basev0.Configuration) *basev0.ConfigurationValue { + for _, conf := range confs { + for _, info := range conf.GetInfos() { + if info.GetName() != "docs" { + continue + } + for _, value := range info.GetConfigurationValues() { + if value.GetKey() == "README" { + return value + } + } + } + } + return nil +} + +// TestAnOrdinaryLoadDeliversTheBoundArtifactWithItsEvidence is the seam end to +// end: the CLI's reader supplies the resolver, core reads the declaration +// during profile loading, acquires the bytes once and attaches the identity it +// computed FROM those bytes. +func TestAnOrdinaryLoadDeliversTheBoundArtifactWithItsEvidence(t *testing.T) { + body := "# Install\n\nRun `codefly run`.\n" + workspace := boundWorkspace(t, body, nil) + + value := boundValue(loadBound(t, workspace)) + require.NotNil(t, value, "the bound destination was delivered") + require.Equal(t, body, value.GetValue(), "exact bytes, newlines included") + require.Nil(t, value.GetModuleArtifact(), "the declaration is resolved, not delivered unresolved") + evidence := value.GetResolvedModuleArtifact() + require.NotNil(t, evidence) + require.Equal(t, "wiki", evidence.GetReference().GetModule()) + require.Equal(t, "docs/install.md", evidence.GetReference().GetPath()) + require.Equal(t, int64(len(body)), evidence.GetArtifactIdentity().GetSizeBytes()) + require.NoError(t, resources.ValidateModuleArtifactValue(value)) +} + +// TestANextLoadDeliversChangedSource is the per-load contract, and the reason +// there is no flow-spanning freeze: an earlier observation describes itself, +// and the next supported load delivers what the source says then. +func TestANextLoadDeliversChangedSource(t *testing.T) { + workspace := boundWorkspace(t, "A\n", nil) + require.Equal(t, "A\n", boundValue(loadBound(t, workspace)).GetValue()) + + require.NoError(t, os.WriteFile(filepath.Join(workspace.Dir(), "modules/wiki/docs/install.md"), []byte("B\n"), 0o600)) + later := boundValue(loadBound(t, workspace)) + require.Equal(t, "B\n", later.GetValue(), "the next load acquires the then-selected source") + require.NoError(t, resources.ValidateModuleArtifactValue(later)) +} + +// TestALiteralCannotSilentlyTakeTheBoundDestination is the transition rule: +// moving from a literal to a binding is an explicit reviewed source edit, so +// the two declarations together refuse rather than one quietly winning — +// whichever way precedence happens to fall. +func TestALiteralCannotSilentlyTakeTheBoundDestination(t *testing.T) { + workspace := boundWorkspace(t, "# Install\n", map[string]string{ + "configurations/local/docs.env": "README=a literal nobody removed\n", + }) + reader, err := NewWorkspaceConfigurationReader(context.Background(), workspace) + require.NoError(t, err) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + require.Error(t, reader.Load(context.Background(), env.Runtime())) +} + +// TestAMissingArtifactRefusesTheLoad keeps the failure closed: there is no +// cached literal, per-key fallback or empty success for a declared binding +// whose source is gone. +func TestAMissingArtifactRefusesTheLoad(t *testing.T) { + workspace := boundWorkspace(t, "# Install\n", nil) + require.NoError(t, os.Remove(filepath.Join(workspace.Dir(), "modules/wiki/docs/install.md"))) + + reader, err := NewWorkspaceConfigurationReader(context.Background(), workspace) + require.NoError(t, err) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + err = reader.Load(context.Background(), env.Runtime()) + require.Error(t, err) + require.Contains(t, err.Error(), "wiki", "the refusal names the declared module") + require.Contains(t, err.Error(), "docs/install.md", "and its declared path") +} + +// TestACheckLoadAcquiresItsOwnObservation is the standalone-check rule: it +// uses the same loader and resolver as a run, and records what IT saw — it +// neither inherits a run's acquisition nor proves a later delivery. +func TestACheckLoadAcquiresItsOwnObservation(t *testing.T) { + workspace := boundWorkspace(t, "A\n", nil) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + + checked, _, err := WorkspaceConfigurationsForChecking(context.Background(), workspace, env) + require.NoError(t, err) + value := boundValue([]*basev0.Configuration{{Origin: resources.ConfigurationWorkspace, Infos: checked.Infos}}) + require.NotNil(t, value) + require.Equal(t, "A\n", value.GetValue()) + require.NotNil(t, value.GetResolvedModuleArtifact()) +} + +// TestAnAdditionalOfferCannotTakeABoundDestination is the delivery check at +// the phase seams, exercised through the call the runner and the excluded-root +// export both make. Equality gives no replacement right, an empty override map +// does not skip the configuration-to-configuration comparison, and an +// unrelated offer is untouched. +func TestAnAdditionalOfferCannotTakeABoundDestination(t *testing.T) { + workspace := boundWorkspace(t, "# Install\n", nil) + delivered := detachConfigurations(loadBound(t, workspace)) + require.NotNil(t, boundValue(delivered)) + + offer := func(key, text string) []*basev0.Configuration { + return []*basev0.Configuration{{Origin: resources.ConfigurationWorkspace, Infos: []*basev0.ConfigurationInformation{{ + Name: "docs", ConfigurationValues: []*basev0.ConfigurationValue{{Key: key, Value: text}}, + }}}} + } + + require.NoError(t, resources.ValidateModuleArtifactDelivery(delivered, nil, nil), + "the single legitimate bound value, with no contribution, passes") + require.Error(t, resources.ValidateModuleArtifactDelivery(delivered, offer("README", "something else"), nil), + "a returned or dependency offer for the bound destination refuses with an empty override map") + require.Error(t, resources.ValidateModuleArtifactDelivery(delivered, offer("README", "# Install\n"), nil), + "and copying the bytes buys no replacement authority") + require.NoError(t, resources.ValidateModuleArtifactDelivery(delivered, offer("CHANGELOG", "unrelated"), nil), + "an unrelated key keeps its ordinary behaviour") + + // The override map is compared by carrier, not by value, including the + // hyphen and secret-namespace spellings the SDK accepts. + key := resources.ConfigurationValueEnvironmentKey( + &basev0.Configuration{Origin: resources.ConfigurationWorkspace}, + "docs", &basev0.ConfigurationValue{Key: "README"}) + require.Error(t, resources.ValidateModuleArtifactDelivery(delivered, nil, map[string]string{key: "via --set"}), + "a runtime override for the bound carrier refuses") + require.NoError(t, resources.ValidateModuleArtifactDelivery(delivered, nil, map[string]string{"CODEFLY__UNRELATED": "fine"})) +} + +// TestTheExcludedRootExportChecksItsOwnPayload closes the one export that +// runs no agent Init: there is no returned contribution and no retained +// delivery, so the selected payload is checked against itself and against the +// export's own override map before the first append. +func TestTheExcludedRootExportChecksItsOwnPayload(t *testing.T) { + ctx := context.Background() + workspace := boundWorkspace(t, "# Install\n", nil) + module, err := workspace.LoadModuleFromName(ctx, "wiki") + require.NoError(t, err) + service, err := module.LoadServiceFromName(ctx, "backend") + require.NoError(t, err) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + + flow, err := NewFlow(ctx, workspace, module, service, env, RunMode) + require.NoError(t, err) + output := filepath.Join(t.TempDir(), "runtime.env") + flow.WithRuntimeContext(resources.RuntimeContextNative) + flow.WithExcludeRoot(true) + flow.WithOutputEnv(output) + // An override aimed at the bound carrier: the export must refuse before it + // writes anything at all. + flow.WithOverrides(map[string]map[string]string{ + resources.WithUnique(service).Unique(): { + resources.ConfigurationValueEnvironmentKey( + &basev0.Configuration{Origin: resources.ConfigurationWorkspace}, + "docs", &basev0.ConfigurationValue{Key: "README"}): "substituted", + }, + }) + require.Error(t, flow.exportExcludedOriginEnvironment(ctx)) + _, statErr := os.Stat(output) + require.True(t, os.IsNotExist(statErr), "an absent output stays absent") +} diff --git a/pkg/orchestration/module_artifact_test.go b/pkg/orchestration/module_artifact_test.go new file mode 100644 index 000000000..18d8ff973 --- /dev/null +++ b/pkg/orchestration/module_artifact_test.go @@ -0,0 +1,187 @@ +package orchestration + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + + basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" + "github.com/codefly-dev/core/resources" + "github.com/stretchr/testify/require" +) + +// artifactWorkspace is a one-module composition selected by an explicit +// committed path: a developer checkout, which legitimately has no +// materialization receipt, so acquisition succeeds and its provenance is local +// development evidence rather than release authenticity. +func artifactWorkspace(t *testing.T) *resources.Workspace { + t.Helper() + return writeTempWorkspace(t, map[string]string{ + "workspace.codefly.yaml": `name: artifact-export +layout: modules +modules: + - name: wiki + path: modules/wiki +`, + "modules/wiki/module.codefly.yaml": excludedRootExportModule, + "modules/wiki/services/backend/service.codefly.yaml": excludedRootExportService, + }) +} + +func artifactReference(module, path string) *basev0.ConfigurationModuleArtifactReference { + return &basev0.ConfigurationModuleArtifactReference{Module: module, Path: path} +} + +// TestTheResolverAcquiresTheSelectedModulesExactBytes is the delivery truth: +// whitespace and newlines included, byte for byte, with no reserialization. +func TestTheResolverAcquiresTheSelectedModulesExactBytes(t *testing.T) { + ctx := context.Background() + workspace := artifactWorkspace(t) + body := "# Install\n\n indented\ttab\n\nlast line without newline" + require.NoError(t, os.MkdirAll(filepath.Join(workspace.Dir(), "modules/wiki/docs"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(workspace.Dir(), "modules/wiki/docs/install.md"), []byte(body), 0o600)) + + content, err := ModuleArtifactResolverFor(workspace)(ctx, artifactReference("wiki", "docs/install.md")) + require.NoError(t, err) + require.Equal(t, body, string(content)) +} + +// TestTheResolverRefusesWhatItCannotConfineOrBound covers the acquisition +// obligations core hands to the CLI. Every one of these is a real file or a +// real path, not a stub: a string prefix check followed by an unrestricted +// open passes several of them. +func TestTheResolverRefusesWhatItCannotConfineOrBound(t *testing.T) { + ctx := context.Background() + workspace := artifactWorkspace(t) + module := filepath.Join(workspace.Dir(), "modules/wiki") + require.NoError(t, os.MkdirAll(filepath.Join(module, "docs"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(module, "docs/install.md"), []byte("ok\n"), 0o600)) + + // A secret parked beside the composition, which a traversal or a symlink + // out of the module would otherwise deliver as public configuration. + require.NoError(t, os.WriteFile(filepath.Join(workspace.Dir(), "outside.txt"), []byte("not-public\n"), 0o600)) + require.NoError(t, os.Symlink(filepath.Join(workspace.Dir(), "outside.txt"), filepath.Join(module, "docs/escape.md"))) + require.NoError(t, os.Symlink("../../../outside.txt", filepath.Join(module, "docs/relative-escape.md"))) + require.NoError(t, syscall.Mkfifo(filepath.Join(module, "docs/pipe"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(module, "docs/oversize.md"), + []byte(strings.Repeat("x", resources.FileCarrierThreshold+1)), 0o600)) + + resolve := ModuleArtifactResolverFor(workspace) + for _, probe := range []struct { + name string + ref *basev0.ConfigurationModuleArtifactReference + }{ + {"an absolute path", artifactReference("wiki", filepath.Join(workspace.Dir(), "outside.txt"))}, + {"a traversing path", artifactReference("wiki", "../../outside.txt")}, + {"an absolute symlink out of the module", artifactReference("wiki", "docs/escape.md")}, + {"a relative symlink out of the module", artifactReference("wiki", "docs/relative-escape.md")}, + {"a FIFO, which would block the read", artifactReference("wiki", "docs/pipe")}, + {"a directory", artifactReference("wiki", "docs")}, + {"a missing file", artifactReference("wiki", "docs/absent.md")}, + {"an empty path", artifactReference("wiki", "")}, + {"a module the composition does not name", artifactReference("ghost", "docs/install.md")}, + } { + t.Run(probe.name, func(t *testing.T) { + content, err := resolve(ctx, probe.ref) + require.Error(t, err) + require.Empty(t, content) + require.NotContains(t, err.Error(), "not-public", "the refusal never carries the content it refused") + }) + } + + // One byte over core's inclusive bound is acquired, then refused by core's + // own byte admission — the CLI reads the extra byte precisely so the + // refusal can be made rather than silently truncating. + oversize, err := resolve(ctx, artifactReference("wiki", "docs/oversize.md")) + require.NoError(t, err) + require.Len(t, oversize, resources.FileCarrierThreshold+1) + require.Error(t, resources.ValidateModuleArtifactBytes(string(oversize))) + + // And the inclusive threshold itself is admitted. + require.NoError(t, os.WriteFile(filepath.Join(module, "docs/exact.md"), + []byte(strings.Repeat("x", resources.FileCarrierThreshold)), 0o600)) + exact, err := resolve(ctx, artifactReference("wiki", "docs/exact.md")) + require.NoError(t, err) + require.NoError(t, resources.ValidateModuleArtifactBytes(string(exact))) +} + +// TestTheResolverRefusesAnUnmaterializedModule is the receipt half seen from +// the consuming side: a composed module this machine has not materialized is +// refused rather than fetched behind the read, and no literal stands in. +func TestTheResolverRefusesAnUnmaterializedModule(t *testing.T) { + ctx := context.Background() + workspace := writeTempWorkspace(t, map[string]string{ + "workspace.codefly.yaml": `name: artifact-pinned +layout: modules +modules: + - name: wiki + source: github.com/codefly-dev/absent + version: 0.0.1 +`, + }) + content, err := ModuleArtifactResolverFor(workspace)(ctx, artifactReference("wiki", "docs/install.md")) + require.Error(t, err) + require.Empty(t, content) +} + +// TestTheDeliveryBaselinePreservesEveryOffer is what makes core's collision +// check possible: the offer precedence would discard is exactly the one that +// must still be visible, so nothing is merged, deduplicated or keyed first. +func TestTheDeliveryBaselinePreservesEveryOffer(t *testing.T) { + value := func(key, text string) *basev0.ConfigurationValue { + return &basev0.ConfigurationValue{Key: key, Value: text} + } + own := &basev0.Configuration{Origin: "wiki/backend", Infos: []*basev0.ConfigurationInformation{{ + Name: "docs", ConfigurationValues: []*basev0.ConfigurationValue{value("README", "a")}, + }}} + first := &basev0.Configuration{Origin: resources.ConfigurationWorkspace, Infos: []*basev0.ConfigurationInformation{{ + Name: "docs", ConfigurationValues: []*basev0.ConfigurationValue{value("README", "b")}, + }}} + // The same pointer offered twice, and a duplicate coordinate: both survive. + baseline := deliveryBaseline(own, []*basev0.Configuration{first, first}, []*basev0.Configuration{first}) + require.Len(t, baseline, 4) + require.Same(t, own, baseline[0]) + for _, conf := range baseline[1:] { + require.Same(t, first, conf) + } + require.Empty(t, deliveryBaseline(nil, nil, nil)) +} + +// TestTheRetainedDeliveryIsDetached locks what Start compares against. The +// retained payload must not move when the slices it was gathered from are +// mutated by a later load, and the generated artifact evidence must travel +// with each value — a copy that dropped it would protect nothing. +func TestTheRetainedDeliveryIsDetached(t *testing.T) { + body := "# Install\n" + digest := sha256.Sum256([]byte(body)) + evidence := &basev0.ResolvedConfigurationModuleArtifact{ + Reference: artifactReference("wiki", "docs/install.md"), + ArtifactIdentity: &basev0.SourceIdentity{ + Algorithm: basev0.SourceIdentityAlgorithm_SOURCE_IDENTITY_ALGORITHM_SHA256, + Digest: hex.EncodeToString(digest[:]), + SizeBytes: int64(len(body)), + }, + } + source := []*basev0.Configuration{nil, {Origin: resources.ConfigurationWorkspace, Infos: []*basev0.ConfigurationInformation{{ + Name: "docs", ConfigurationValues: []*basev0.ConfigurationValue{ + {Key: "README", Value: body, ResolvedModuleArtifact: evidence}, + }, + }}}} + + detached := detachConfigurations(source) + require.Len(t, detached, 1, "a nil offer carries nothing to retain") + retained := detached[0].Infos[0].ConfigurationValues[0] + require.Equal(t, body, retained.Value) + require.NotNil(t, retained.ResolvedModuleArtifact, "the evidence travels with the copy") + require.NotSame(t, evidence, retained.ResolvedModuleArtifact, "and it is a clone, not the same message") + require.NoError(t, resources.ValidateModuleArtifactValue(retained)) + + source[1].Infos[0].ConfigurationValues[0].Value = "rewritten" + require.Equal(t, body, detached[0].Infos[0].ConfigurationValues[0].Value, + "the retained delivery is what the service received, whatever happens to the slices it came from") +} diff --git a/pkg/orchestration/output_records.go b/pkg/orchestration/output_records.go new file mode 100644 index 000000000..ff504a0c5 --- /dev/null +++ b/pkg/orchestration/output_records.go @@ -0,0 +1,109 @@ +package orchestration + +import ( + "fmt" + "strings" + + "github.com/codefly-dev/core/resources" +) + +// outputRecord is one line of the `--output-env` artifact, already serialized. +// +// The string is converted ONCE, here, and carried through validation to the +// write: a value is an `any`, so converting it again at write time could +// produce a different string from the one that was checked — which is the +// whole hole a format check is supposed to close. +type outputRecord struct { + key string + line string +} + +// outputRecords converts an emitter's variables into serialized records and +// holds the complete batch to the format the supported reader actually parses. +// +// The reader (sdk-go LoadRuntimeEnvironmentFile) scans lines, strips one +// trailing CR, cuts at the FIRST `=`, and refuses the whole file if any name +// is not `CODEFLY__` plus at least one character of [A-Z0-9_]. So this is +// transport grammar, not a second semantic carrier matcher, and it applies to +// EVERY record in the batch — bound artifact, ordinary value, override, +// identity, fixture, endpoint alike. A value carrying LF is the reason: the +// reader would read its tail as another record, and `NOTE=note\nCODEFLY__…=B` +// replaces a value no key matching could have protected. CR is refused +// because the reader silently eats a trailing one; NUL because a process +// environment cannot carry it. +// +// Diagnostics name a record's index and a generic reason, or a key that is +// already known to be well formed. A malformed key and every value stay out of +// the message: they are the attacker-controlled halves. +func outputRecords(environments []*resources.EnvironmentVariable) ([]outputRecord, error) { + records := make([]outputRecord, 0, len(environments)) + for index, env := range environments { + if env == nil { + return nil, fmt.Errorf("output environment record %d is empty", index) + } + if err := validOutputKey(env.Key); err != nil { + return nil, fmt.Errorf("output environment record %d: %w", index, err) + } + // resources.EnvironmentVariable.String is this same conversion; it is + // spelled out so the retained line and the checked line are one value. + line := fmt.Sprintf("%s=%v", env.Key, env.Value) + if err := validOutputValue(line[len(env.Key)+1:]); err != nil { + return nil, fmt.Errorf("output environment carrier %s: %w", env.Key, err) + } + records = append(records, outputRecord{key: env.Key, line: line}) + } + return records, nil +} + +// validOutputKey is the reader's own name grammar, byte for byte. +func validOutputKey(key string) error { + const prefix = "CODEFLY__" + if !strings.HasPrefix(key, prefix) || len(key) == len(prefix) { + return fmt.Errorf("its name is not a CODEFLY__ carrier, so the supported reader refuses the whole artifact") + } + for index := 0; index < len(key); index++ { + character := key[index] + if (character < 'A' || character > 'Z') && (character < '0' || character > '9') && character != '_' { + return fmt.Errorf("its name is not uppercase ASCII, digits and underscores, so the supported reader refuses the whole artifact") + } + } + return nil +} + +// validOutputValue refuses the delimiters this format cannot represent. An +// empty value and a value holding one or many `=` are representable: the +// reader cuts at the first separator and keeps the rest verbatim. +func validOutputValue(value string) error { + if strings.ContainsAny(value, "\n\r\x00") { + return fmt.Errorf("its value contains a line break or NUL, which this raw output format cannot represent; direct process delivery carries it unchanged") + } + return nil +} + +// validRetainedOutputRecords refuses an existing artifact this writer would +// otherwise rewrite line by line. A retained line is not authenticated source +// evidence — syntax says nothing about where a well formed old record came +// from — but rewriting a malformed one would launder it into output this run +// is answerable for. +func validRetainedOutputRecords(existing string) error { + for number, line := range strings.Split(existing, "\n") { + if line == "" { + continue + } + key, value, found := strings.Cut(line, "=") + if !found { + return fmt.Errorf("the existing output environment holds a record at line %d with no separator", number+1) + } + if err := validOutputKey(key); err != nil { + return fmt.Errorf("the existing output environment holds a record at line %d whose name is not a supported carrier", number+1) + } + // The value too, not just the name: the rewrite reproduces this line + // verbatim, and a retained value carrying CR or NUL is exactly as + // unreadable as a new one. Splitting on LF cannot leave an embedded LF + // here, but CR and NUL survive it. + if err := validOutputValue(value); err != nil { + return fmt.Errorf("the existing output environment holds a record at line %d whose value this format cannot represent", number+1) + } + } + return nil +} diff --git a/pkg/orchestration/output_records_test.go b/pkg/orchestration/output_records_test.go new file mode 100644 index 000000000..1cb3dbe0f --- /dev/null +++ b/pkg/orchestration/output_records_test.go @@ -0,0 +1,234 @@ +package orchestration + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" + "github.com/codefly-dev/core/resources" +) + +// variable is one record offered to the writer. +func variable(key string, value any) *resources.EnvironmentVariable { + return &resources.EnvironmentVariable{Key: key, Value: value} +} + +// TestTheOutputWriterRefusesRecordsTheReaderCannotParse is the counterexample +// the whole-record rule exists for: an unrelated, UNBOUND value whose text +// carries an LF and a second `KEY=value` line. Semantic carrier matching +// cannot see it — the record's own key is nothing special — but the reader +// scans lines, so that tail lands in the process environment as a value for a +// key the run never offered. Refusing every unrepresentable record, bound or +// not, is what closes it. +func TestTheOutputWriterRefusesRecordsTheReaderCannotParse(t *testing.T) { + ctx := context.Background() + for _, probe := range []struct { + name string + records []*resources.EnvironmentVariable + }{ + {"a line break smuggles a second record", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE", "note\nCODEFLY__WORKSPACE_CONFIGURATION__DOCS__README=substituted"), + }}, + {"a carriage return the reader would silently eat", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE", "note\r"), + }}, + {"a NUL a process environment cannot carry", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE", "note\x00tail"), + }}, + {"a key outside the reader's prefix", []*resources.EnvironmentVariable{ + variable("NOTE", "plain"), + }}, + {"a key with no suffix at all", []*resources.EnvironmentVariable{ + variable("CODEFLY__", "plain"), + }}, + {"a lowercase key", []*resources.EnvironmentVariable{ + variable("CODEFLY__note", "plain"), + }}, + {"a hyphenated key", []*resources.EnvironmentVariable{ + variable("CODEFLY__READ-WRITE", "plain"), + }}, + {"a key carrying the separator", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE=X", "plain"), + }}, + {"a key carrying a line break", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE\nCODEFLY__OTHER", "plain"), + }}, + // The offending record is LAST, which is the point: a check that ran + // per append would already have written the valid one. + {"an invalid record after a valid one", []*resources.EnvironmentVariable{ + variable("CODEFLY__WORKSPACE_CONFIGURATION__DOCS__README", "fine"), + variable("CODEFLY__NOTE", "note\nCODEFLY__WORKSPACE_CONFIGURATION__DOCS__README=substituted"), + }}, + } { + t.Run(probe.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "runtime.env") + require.Error(t, appendEnvironmentVariablesToFile(ctx, path, probe.records)) + _, err := os.Stat(path) + require.True(t, os.IsNotExist(err), "an absent output stays absent") + + // And an existing artifact keeps its exact bytes. + existing := filepath.Join(t.TempDir(), "runtime.env") + require.NoError(t, appendEnvironmentVariablesToFile(ctx, existing, []*resources.EnvironmentVariable{ + variable("CODEFLY__SENTINEL", "untouched"), + })) + before, err := os.ReadFile(existing) + require.NoError(t, err) + require.Error(t, appendEnvironmentVariablesToFile(ctx, existing, probe.records)) + after, err := os.ReadFile(existing) + require.NoError(t, err) + require.Equal(t, before, after, "a refused batch leaves the existing artifact byte-identical") + }) + } +} + +// TestRefusalDiagnosticsCarryNoValuesOrMalformedKeys keeps the refusal from +// becoming the leak: a malformed key and every value are the halves an +// attacker controls, so the message names a record index or an already valid +// carrier and a generic reason. +func TestRefusalDiagnosticsCarryNoValuesOrMalformedKeys(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "runtime.env") + + err := appendEnvironmentVariablesToFile(ctx, path, []*resources.EnvironmentVariable{ + variable("CODEFLY__FINE", "ok"), + variable("totally-made-up-key", "secret-value"), + }) + require.Error(t, err) + require.Contains(t, err.Error(), "record 1") + require.NotContains(t, err.Error(), "totally-made-up-key") + require.NotContains(t, err.Error(), "secret-value") + + err = appendEnvironmentVariablesToFile(ctx, path, []*resources.EnvironmentVariable{ + variable("CODEFLY__TOKEN", "line\nsecret-value"), + }) + require.Error(t, err) + require.Contains(t, err.Error(), "CODEFLY__TOKEN") + require.NotContains(t, err.Error(), "secret-value") +} + +// readerGrammar is sdk-go v0.4.0's validRuntimeEnvironmentName, transcribed +// for this test ONLY. +// +// The CLI does not depend on root sdk-go and this must not make it: the +// production check lives in validOutputKey, and the agreement between the two +// is itself asserted below, so a divergence shows up here rather than in a +// run. A roundtrip through the real reader is a separate, out-of-process +// check against a built binary. +func readerGrammar(name string) bool { + if !strings.HasPrefix(name, "CODEFLY__") || len(name) == len("CODEFLY__") { + return false + } + for _, character := range name { + switch { + case character >= 'A' && character <= 'Z': + case character >= '0' && character <= '9': + case character == '_': + default: + return false + } + } + return true +} + +// TestTheWriterAgreesWithTheReadersGrammar is what keeps the production rule +// honest: every name the writer admits the reader must admit, and the other +// way round. +func TestTheWriterAgreesWithTheReadersGrammar(t *testing.T) { + for _, name := range []string{ + "CODEFLY__WORKSPACE_CONFIGURATION__DOCS__README", "CODEFLY__A", "CODEFLY__9", "CODEFLY___", + "CODEFLY__", "CODEFLY", "codefly__A", "CODEFLY__a", "CODEFLY__A-B", "CODEFLY__A B", + "CODEFLY__A=B", "CODEFLY__A\nB", "CODEFLY__Ä", "", "NOTE", + resources.ConfigurationDocumentKey(resources.ConfigurationWorkspace, "fixture-composition", "local", false), + } { + require.Equal(t, readerGrammar(name), validOutputKey(name) == nil, + "the writer and the supported reader disagree about a carrier name") + } +} + +// TestRepresentableRecordsRoundTrip is the positive half: an empty value and +// a value holding several `=` survive unchanged, because the reader cuts at +// the FIRST separator only. +func TestRepresentableRecordsRoundTrip(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "runtime.env") + require.NoError(t, appendEnvironmentVariablesToFile(ctx, path, []*resources.EnvironmentVariable{ + variable("CODEFLY__WORKSPACE_CONFIGURATION__DOCS__README", "k=v=w"), + variable("CODEFLY__EMPTY", ""), + variable("CODEFLY__DIGITS_9", "9"), + })) + body, err := os.ReadFile(path) + require.NoError(t, err) + values := map[string]string{} + for _, line := range strings.Split(strings.TrimSuffix(string(body), "\n"), "\n") { + name, value, found := strings.Cut(strings.TrimSuffix(line, "\r"), "=") + require.True(t, found) + require.True(t, readerGrammar(name)) + values[name] = value + } + require.Equal(t, map[string]string{ + "CODEFLY__WORKSPACE_CONFIGURATION__DOCS__README": "k=v=w", + "CODEFLY__EMPTY": "", + "CODEFLY__DIGITS_9": "9", + }, values) +} + +// TestTheWriterRefusesMalformedRetainedRecords covers the rewrite direction. +// The writer rereads the file and rewrites it whole, so a malformed line +// already there would be reproduced under this run's name. Syntax says +// nothing about where a well formed old record came from — this is integrity, +// not provenance — but laundering a record the reader refuses is still the +// writer's doing. +func TestTheWriterRefusesMalformedRetainedRecords(t *testing.T) { + ctx := context.Background() + for _, retained := range []string{ + "NOTE=plain\n", + "CODEFLY__lower=plain\n", + "no-separator-at-all\n", + } { + path := filepath.Join(t.TempDir(), "runtime.env") + require.NoError(t, os.WriteFile(path, []byte(retained), 0o600)) + require.Error(t, appendEnvironmentVariablesToFile(ctx, path, []*resources.EnvironmentVariable{ + variable("CODEFLY__FINE", "ok"), + })) + body, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, retained, string(body), "the refused rewrite left the file alone") + } +} + +// TestAStructuredConfigurationDocumentCannotBeExportedRaw records a defect +// this format rule surfaced rather than created, and pins the CLI's honest +// half of it. +// +// resources.ConfigurationDocumentKey is a prefix plus a LOWERCASE hex SHA256, +// and sdk-go's validRuntimeEnvironmentName accepts only A–Z, 0–9 and `_`. So +// the reader refuses the WHOLE artifact whenever a run exports a structured +// configuration document — platform-core's merged fixture-composition is +// exactly that shape. Before this check the CLI wrote such a file and +// reported success; nothing could ever load it. Refusing before the first +// write is the only answer available here: making it exportable needs either +// an uppercase document carrier in core or a reader in sdk-go that accepts +// this one, and neither belongs to the CLI. +func TestAStructuredConfigurationDocumentCannotBeExportedRaw(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "runtime.env") + native := resources.NewRuntimeContextNative() + document := &basev0.Configuration{Origin: resources.ConfigurationWorkspace, RuntimeContext: native, Infos: []*basev0.ConfigurationInformation{{ + Name: "fixture-composition", + Data: &basev0.ConfigurationData{Kind: "yaml", Content: []byte("schema: codefly.dev/authority/fixture-composition/v1\nroles: []\n")}, + }}} + key := resources.ConfigurationDocumentKey(resources.ConfigurationWorkspace, "fixture-composition", "local", false) + require.True(t, strings.HasPrefix(key, "CODEFLY__"), "the prefix is fine; the digest is not") + require.Error(t, validOutputKey(key), "the reader's own grammar refuses a lowercase hex digest") + + err := AppendServiceProcessConfigurationsToFile(ctx, path, localRunEnvironment(), native, nil, + []*basev0.Configuration{document}, nil, nil) + require.Error(t, err) + _, statErr := os.Stat(path) + require.True(t, os.IsNotExist(statErr), "nothing was written") +} diff --git a/pkg/orchestration/runner.go b/pkg/orchestration/runner.go index 65b81118b..64f2652b0 100644 --- a/pkg/orchestration/runner.go +++ b/pkg/orchestration/runner.go @@ -71,6 +71,16 @@ type Runner struct { // state says it does (addressesMoved). startedMappings string + // delivered is the configuration payload this service's last SUCCESSFUL + // Init actually received, detached from the slices it was gathered from. + // Start compares its own offers against it (module_artifact.go): a + // destination a module artifact was bound to at Init cannot be taken over + // later by a dependency's or an override's value for the same carrier. + // Nil until an Init succeeds — a failed one leaves no baseline rather than + // an empty one that would protect nothing — and replaced whole by the next + // successful Init, which is the ordinary way a reload adopts new bytes. + delivered []*basev0.Configuration + restartMu sync.Mutex pendingRestart ActionType restartInFlight bool @@ -294,13 +304,6 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { if err = runner.checkInitialPortAvailability(ctx, inputs.networkMappings); err != nil { return nil, w.Wrapf(err, "cannot initialize %s", runner.instance.Unique()) } - // A container-only service with a build recipe gets the image this machine - // just built, as a canonical digest reference; everything else keeps the - // agent's own image selection and hands Init an empty field. - runtimeImage, err := runner.localRuntimeImage(ctx) - if err != nil { - return nil, w.Wrapf(err, "cannot prepare local runtime image for %s", runner.instance.Unique()) - } dependenciesEndpoints := inputs.dependenciesEndpoints dependenciesNetworkMappings := inputs.dependenciesNetworkMappings conf := inputs.conf @@ -309,6 +312,47 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { dependenciesConfigurations := inputs.dependenciesConfigurations networkMappings := inputs.networkMappings + // The admitted payload, unmerged, and the process override map, computed + // ONCE: the same map rides the request and is what core compares against, + // so a late callback cannot make the request and the check disagree. Core + // owns the comparison (carriers, aliases, origin/group/key); it refuses an + // override, or a second offer inside this payload, that would take over a + // destination a module artifact is bound to. + overrides := runner.runtimeOverridesFor(networkMappings) + baseline := deliveryBaseline(conf, workspaceConfigurations, dependenciesConfigurations) + if err = resources.ValidateModuleArtifactDelivery(baseline, nil, overrides); err != nil { + return nil, w.Wrapf(err, "cannot initialize %s", runner.instance.Unique()) + } + // Known-invalid output is refused before the agent is called at all, where + // the inputs already exist. This is early refusal, not a substitute for + // checking the response: what the agent returns is unknown until it + // answers, and a refusal then cannot pretend Init had no effects. + if runner.outputEnv != "" { + environment, envErr := runner.world.Env.Proto() + if envErr != nil { + return nil, w.Wrapf(envErr, "cannot name the run environment for the output environment") + } + known, knownErr := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, conf, workspaceConfigurations, nil, nil) + if knownErr != nil { + return nil, knownErr + } + if _, knownErr = outputRecords(known); knownErr != nil { + return nil, w.Wrapf(knownErr, "cannot export the output environment for %s", runner.instance.Unique()) + } + } + + // Only now the image work, which builds and loads: the delivery check above + // has no side effects and must settle first, so a payload this run is going + // to refuse does not leave a built image behind as its only trace. + // + // A container-only service with a build recipe gets the image this machine + // just built, as a canonical digest reference; everything else keeps the + // agent's own image selection and hands Init an empty field. + runtimeImage, err := runner.localRuntimeImage(ctx) + if err != nil { + return nil, w.Wrapf(err, "cannot prepare local runtime image for %s", runner.instance.Unique()) + } + // Init is the only lifecycle call guaranteed to reach a service under test: // a test policy replaces the origin's Start with a barrier or skips it // altogether, so a fixture or an override delivered only at Start reaches @@ -324,7 +368,7 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { WorkspaceConfigurations: workspaceConfigurations, DependenciesConfigurations: dependenciesConfigurations, Fixture: runner.fixture, - Overrides: runner.runtimeOverridesFor(networkMappings), + Overrides: overrides, } err = resources.Validate(req) if err != nil { @@ -368,6 +412,21 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { runner.networkMappings = accepted runner.pinAcceptedPorts(accepted) + // Only a successful Init establishes a delivered payload, and the retained + // copy is detached: the configurations this service actually received stay + // what they were whatever later loads, filters or clones do to the slices + // they came from. A failed or missing Init leaves no baseline rather than + // an empty one. + runner.delivered = detachConfigurations(baseline) + // The returned configurations are an additional offer, checked BEFORE they + // are exposed or written: an agent that answers with its own value for a + // destination a module artifact is bound to does not get to replace it by + // returning the same bytes, and an empty override map does not skip the + // configuration-to-configuration comparison. + if err = resources.ValidateModuleArtifactDelivery(runner.delivered, resp.RuntimeConfigurations, overrides); err != nil { + return nil, w.Wrapf(err, "cannot accept the configurations %s returned from its initialization", runner.instance.Unique()) + } + err = runner.world.ConfigurationManager.ExposeConfiguration(ctx, runner.instance.Identity, resp.RuntimeConfigurations...) if err != nil { return nil, w.Wrapf(err, "cannot record shared configuration infos") @@ -384,17 +443,20 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { if envErr != nil { return nil, w.Wrapf(envErr, "cannot name the run environment for the output environment") } - err = AppendServiceProcessConfigurationsToFile( - ctx, - runner.outputEnv, - environment, - runtimeContext, - conf, - workspaceConfigurations, - nil, - resp.RuntimeConfigurations, - ) - if err != nil { + // One chunk, so the exported slice and the validated batch are the same + // projection: the admitted own/workspace values this run already + // checked, plus what the agent returned. The full admitted payload + // remains the collision baseline above — including bound inputs this + // export omits — rather than being re-offered here as a contribution. + variables, variablesErr := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, conf, workspaceConfigurations, nil, resp.RuntimeConfigurations) + if variablesErr != nil { + return nil, variablesErr + } + records, recordsErr := outputRecords(variables) + if recordsErr != nil { + return nil, w.Wrapf(recordsErr, "cannot export the output environment for %s", runner.instance.Unique()) + } + if err = writeOutputRecords(ctx, runner.outputEnv, records); err != nil { return nil, w.Wrapf(err, "cannot write environment variables to file") } } @@ -917,6 +979,88 @@ func (runner *Runner) InitRemote(ctx context.Context) (*OutputProperty, error) { return runner.outputPropertyForInit.Process(ctx) } +// pendingStartOutput converts everything this Start would export, in the +// order the file has always carried it, and returns it together with the +// configuration offers that are new at this phase — without writing anything. +// Gathering it up front is what lets a refusal leave the output untouched: an +// invalid record in the second chunk must not be found after the first has +// reached disk. It answers (nil, nil, nil) when no output was requested. +func (runner *Runner) pendingStartOutput( + ctx context.Context, + dependenciesNetworkMappings []*basev0.NetworkMapping, + overrides map[string]string, +) ([]outputRecord, []*basev0.Configuration, error) { + if runner.outputEnv == "" { + return nil, nil, nil + } + w := wool.Get(ctx).In("service.NewRunner", wool.ThisField(runner.instance)) + runtimeContext, runtimeErr := resources.NewRuntimeContext(runner.runtimeContext) + if runtimeErr != nil { + return nil, nil, w.Wrapf(runtimeErr, "cannot create output environment runtime context") + } + // Environment export needs the same identity fields the agent receives, + // but it must also work for infrastructure agents whose optional path + // fields are empty. ServiceIdentity.Proto validates those deployment + // fields and is therefore intentionally not the conversion boundary. + identity := &basev0.ServiceIdentity{ + Workspace: runner.instance.Identity.Workspace, + Module: runner.instance.Identity.Module, + Name: runner.instance.Identity.Name, + Version: runner.instance.Identity.Version, + WorkspacePath: runner.instance.Identity.WorkspacePath, + RelativeToWorkspace: runner.instance.Identity.RelativeToWorkspace, + } + // Core guards this same dereference before making the identical call + // (services.RuntimeInstance.Start) and simply forwards the request + // unnarrowed when the module is absent. This carrier cannot do that: + // unnarrowed here means writing dependency addresses whose visibility + // nothing can check. Without a consumer module there is no permitted + // set to compute, so refuse to write rather than write unchecked. + if runner.instance.Module == nil { + return nil, nil, w.NewError("cannot resolve dependency network mappings for output environment: %s has no module", runner.instance.Unique()) + } + endpointMappings, mappingErr := outputEnvNetworkMappings( + runner.world.Workspace, + runner.instance.Module.Name, + runner.instance.Service.ServiceDependencies, + runner.networkMappings, + dependenciesNetworkMappings, + ) + if mappingErr != nil { + return nil, nil, w.Wrapf(mappingErr, "cannot resolve dependency network mappings for output environment") + } + // Dependency agents publish connection configurations during their Init. + // Start is the single point that writes them to the output environment, + // after the dependency barrier: the target's own Init may have observed + // the shared configuration state before a concurrently initialized + // dependency exposed its runtime values, so only here are they complete. + dependencyConfigurations, dependencyErr := runner.world.SharedState.GetDependentConfigurationsFor(ctx, runner.instance.Identity) + if dependencyErr != nil { + return nil, nil, w.Wrapf(dependencyErr, "cannot load dependency configurations for output environment") + } + environment, envErr := runner.world.Env.Proto() + if envErr != nil { + return nil, nil, w.Wrapf(envErr, "cannot name the run environment for the output environment") + } + // Both chunks are converted before either is written, in the order the + // file has always carried them: the dependency configurations, then the + // runtime identity, overrides and endpoints. An invalid record in the + // second one must not be discovered after the first has reached disk. + configurationVariables, variablesErr := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, nil, nil, dependencyConfigurations, nil) + if variablesErr != nil { + return nil, nil, variablesErr + } + runtimeVariables, runtimeVariablesErr := runtimeEnvironmentVariables(ctx, identity, runtimeContext, runner.fixture, overrides, endpointMappings) + if runtimeVariablesErr != nil { + return nil, nil, runtimeVariablesErr + } + batch, recordsErr := outputRecords(append(configurationVariables, runtimeVariables...)) + if recordsErr != nil { + return nil, nil, w.Wrapf(recordsErr, "cannot export the output environment for %s", runner.instance.Unique()) + } + return batch, dependencyConfigurations, nil +} + func (runner *Runner) Start(ctx context.Context) (*OutputProperty, error) { w := wool.Get(ctx).In("service.NewRunner", wool.ThisField(runner.instance)) w.Debug("start") @@ -938,95 +1082,60 @@ func (runner *Runner) Start(ctx context.Context) (*OutputProperty, error) { } } - err := runner.StopIfNeeded(ctx) - if err != nil { - return nil, w.Wrapf(err, "cannot stopAfter service instance if needed") + // A nil baseline is not "nothing to protect": Core reads it as an empty + // set of bound destinations and passes, so a Start that never had a + // successful Init would check this phase's offers against nothing. The + // missing evidence is an orchestration error, caught here rather than + // converted into an empty comparison — and after BeginRuntimeStart, so the + // attempt still completes as not-started. + if runner.delivered == nil { + return nil, w.NewError("cannot start %s: no successful initialization retained its configuration delivery, so this phase's configuration offers and overrides cannot be checked against what the service was admitted with", runner.instance.Unique()) } - // Build the request + // Build the request. Everything this phase will deliver is gathered and + // checked BEFORE the running instance is stopped and before either output + // append: a refusal then leaves an absent output absent and an existing + // one byte-identical, where a refusal discovered between the two appends + // could not. These reads are of shared dependency state, which stopping + // this service does not change. dependenciesNetworkMappings, err := runner.world.SharedState.GetDependenciesNetworkMappings(ctx, runner.instance.Service) if err != nil { return nil, w.Wrapf(err, "cannot load service instance") } - if runner.outputEnv != "" { - runtimeContext, runtimeErr := resources.NewRuntimeContext(runner.runtimeContext) - if runtimeErr != nil { - return nil, w.Wrapf(runtimeErr, "cannot create output environment runtime context") - } - // Environment export needs the same identity fields the agent receives, - // but it must also work for infrastructure agents whose optional path - // fields are empty. ServiceIdentity.Proto validates those deployment - // fields and is therefore intentionally not the conversion boundary. - identity := &basev0.ServiceIdentity{ - Workspace: runner.instance.Identity.Workspace, - Module: runner.instance.Identity.Module, - Name: runner.instance.Identity.Name, - Version: runner.instance.Identity.Version, - WorkspacePath: runner.instance.Identity.WorkspacePath, - RelativeToWorkspace: runner.instance.Identity.RelativeToWorkspace, - } - // Core guards this same dereference before making the identical call - // (services.RuntimeInstance.Start) and simply forwards the request - // unnarrowed when the module is absent. This carrier cannot do that: - // unnarrowed here means writing dependency addresses whose visibility - // nothing can check. Without a consumer module there is no permitted - // set to compute, so refuse to write rather than write unchecked. - if runner.instance.Module == nil { - return nil, w.NewError("cannot resolve dependency network mappings for output environment: %s has no module", runner.instance.Unique()) - } - endpointMappings, mappingErr := outputEnvNetworkMappings( - runner.world.Workspace, - runner.instance.Module.Name, - runner.instance.Service.ServiceDependencies, - runner.networkMappings, - dependenciesNetworkMappings, - ) - if mappingErr != nil { - return nil, w.Wrapf(mappingErr, "cannot resolve dependency network mappings for output environment") - } - // Dependency agents publish connection configurations during their Init. - // Start is the single point that writes them to the output environment, - // after the dependency barrier: the target's own Init may have observed - // the shared configuration state before a concurrently initialized - // dependency exposed its runtime values, so only here are they complete. - dependencyConfigurations, dependencyErr := runner.world.SharedState.GetDependentConfigurationsFor(ctx, runner.instance.Identity) - if dependencyErr != nil { - return nil, w.Wrapf(dependencyErr, "cannot load dependency configurations for output environment") - } - environment, envErr := runner.world.Env.Proto() - if envErr != nil { - return nil, w.Wrapf(envErr, "cannot name the run environment for the output environment") - } - err = AppendServiceProcessConfigurationsToFile( - ctx, - runner.outputEnv, - environment, - runtimeContext, - nil, - nil, - dependencyConfigurations, - nil, - ) - if err != nil { - return nil, w.Wrapf(err, "cannot write dependency configurations to output environment") - } - if appendErr := AppendRuntimeEnvironmentToFile( - ctx, - runner.outputEnv, - identity, - runtimeContext, - runner.fixture, - runner.runtimeOverrides(), - endpointMappings, - ); appendErr != nil { - return nil, w.Wrapf(appendErr, "cannot write runtime environment variables to file") + // Computed once and reused for the delivery check, both appends and the + // StartRequest: the override callbacks derive addresses from shared state + // as it stands now, so asking twice can legitimately give two maps. + overrides := runner.runtimeOverrides() + pending, contributions, err := runner.pendingStartOutput(ctx, dependenciesNetworkMappings, overrides) + if err != nil { + return nil, err + } + + // The retained Init payload is the comparison baseline, not a re-emission: + // Start sends no fresh configuration in its request and exports only the + // dependency batch, so a dependency offer that aliases a destination a + // module artifact was bound to at Init refuses here — byte-equal or not. + if err = resources.ValidateModuleArtifactDelivery(runner.delivered, contributions, overrides); err != nil { + return nil, w.Wrapf(err, "cannot start %s", runner.instance.Unique()) + } + + err = runner.StopIfNeeded(ctx) + if err != nil { + return nil, w.Wrapf(err, "cannot stopAfter service instance if needed") + } + + if len(pending) > 0 { + // The same strings that were checked, written through the one writer + // in a single pass rather than rebuilt as two appends. + if err = writeOutputRecords(ctx, runner.outputEnv, pending); err != nil { + return nil, w.Wrapf(err, "cannot write the output environment for %s", runner.instance.Unique()) } } req := &runtimev0.StartRequest{ DependenciesNetworkMappings: dependenciesNetworkMappings, Fixture: runner.fixture, - Overrides: runner.runtimeOverrides(), + Overrides: overrides, } err = resources.Validate(req) if err != nil { @@ -1849,6 +1958,14 @@ func (runner *Runner) WithRemote(environment *environments.Environment) { func AppendEnvironmentVariablesToFile(ctx context.Context, filePath string, environment *basev0.Environment, confs []*basev0.Configuration) error { w := wool.Get(ctx).In("resources.AppendToFile", wool.Field("filePath", filePath)) + // On the UNFILTERED payload, before the filter and before the manager's + // lossy merge: filtering drops offers, and a dropped offer is one the + // collision check can no longer see — while the merge lets an ordinary + // value for a bound destination overwrite it. Same floor as + // serviceProcessConfigurationVariables, for the other exported entrypoint. + if err := resources.ValidateModuleArtifactDelivery(confs, nil, nil); err != nil { + return w.Wrap(err) + } // filter out for native filtered := resources.FilterConfigurations(confs, resources.NewRuntimeContextNative()) m := resources.NewEnvironmentVariableManager() @@ -1893,29 +2010,69 @@ func AppendServiceProcessConfigurationsToFile( dependencyConfigurations []*basev0.Configuration, runtimeConfigurations []*basev0.Configuration, ) error { - w := wool.Get(ctx).In("resources.AppendServiceProcessConfigurationsToFile", wool.Field("filePath", filePath)) + environments, err := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, serviceConfiguration, workspaceConfigurations, dependencyConfigurations, runtimeConfigurations) + if err != nil { + return err + } + return appendEnvironmentVariablesToFile(ctx, filePath, environments) +} + +// serviceProcessConfigurationVariables is that export's conversion, without +// its write: a phase that appends more than one chunk must hold the complete +// batch before the first of them reaches the file, so the conversion and the +// write are separable (output_records.go says why the batch is the unit). +func serviceProcessConfigurationVariables( + ctx context.Context, + environment *basev0.Environment, + runtimeContext *basev0.RuntimeContext, + serviceConfiguration *basev0.Configuration, + workspaceConfigurations []*basev0.Configuration, + dependencyConfigurations []*basev0.Configuration, + runtimeConfigurations []*basev0.Configuration, +) ([]*resources.EnvironmentVariable, error) { + w := wool.Get(ctx).In("resources.AppendServiceProcessConfigurationsToFile") if environment == nil || environment.GetName() == "" { - return w.NewError("the service process environment names no run environment, so a configuration document could not be carried under the key its reader derives") + return nil, w.NewError("the service process environment names no run environment, so a configuration document could not be carried under the key its reader derives") + } + // Before anything is filtered, merged or converted: this is the exported + // entrypoint, so a direct caller reaches it without the phase checks in + // Init and Start, and the manager below is a lossy merge — it appends + // configurations and emits one record per carrier, so an ordinary value + // for a destination a module artifact is bound to simply overwrites it and + // the file ends up with the wrong bytes under the right key. + // + // The complete payload is passed as the comparison baseline with no + // contributions: that checks every offer against every other, which is the + // collision this seam can actually see, without inventing a second offer by + // passing the same values twice. The phases still make their own + // baseline/contribution call with the retained delivery and the phase + // override map; this one is the floor under them, and the record grammar + // check in the writer stays where it is. + if err := resources.ValidateModuleArtifactDelivery( + deliveryBaseline(serviceConfiguration, workspaceConfigurations, append(slices.Clone(dependencyConfigurations), runtimeConfigurations...)), + nil, nil, + ); err != nil { + return nil, w.Wrap(err) } manager := resources.NewEnvironmentVariableManager() manager.SetEnvironment(environment) if err := manager.AddConfigurations(ctx, serviceConfiguration); err != nil { - return w.Wrapf(err, "cannot add service configuration") + return nil, w.Wrapf(err, "cannot add service configuration") } if err := manager.AddConfigurations(ctx, workspaceConfigurations...); err != nil { - return w.Wrapf(err, "cannot add workspace configurations") + return nil, w.Wrapf(err, "cannot add workspace configurations") } if err := manager.AddConfigurations(ctx, resources.FilterConfigurations(dependencyConfigurations, runtimeContext)...); err != nil { - return w.Wrapf(err, "cannot add dependency configurations") + return nil, w.Wrapf(err, "cannot add dependency configurations") } if err := manager.AddConfigurations(ctx, resources.FilterConfigurations(runtimeConfigurations, runtimeContext)...); err != nil { - return w.Wrapf(err, "cannot add runtime configurations") + return nil, w.Wrapf(err, "cannot add runtime configurations") } environments, err := manager.All() if err != nil { - return w.Wrapf(err, "cannot get service process environment variables") + return nil, w.Wrapf(err, "cannot get service process environment variables") } - return appendEnvironmentVariablesToFile(ctx, filePath, environments) + return environments, nil } // outputEnvNetworkMappings is the endpoint set the output environment carries: @@ -2002,9 +2159,28 @@ func AppendRuntimeEnvironmentToFile( overrides map[string]string, endpointMappings []*basev0.NetworkMapping, ) error { - w := wool.Get(ctx).In("resources.AppendRuntimeEnvironmentToFile", wool.Field("filePath", filePath)) + environments, err := runtimeEnvironmentVariables(ctx, identity, runtimeContext, fixture, overrides, endpointMappings) + if err != nil { + return err + } + return appendEnvironmentVariablesToFile(ctx, filePath, environments) +} + +// runtimeEnvironmentVariables is that export's conversion, without its write, +// for the same reason as serviceProcessConfigurationVariables: Start writes +// both chunks, and an invalid record in this one must not be discovered after +// the other has already reached the file. +func runtimeEnvironmentVariables( + ctx context.Context, + identity *basev0.ServiceIdentity, + runtimeContext *basev0.RuntimeContext, + fixture string, + overrides map[string]string, + endpointMappings []*basev0.NetworkMapping, +) ([]*resources.EnvironmentVariable, error) { + w := wool.Get(ctx).In("resources.AppendRuntimeEnvironmentToFile") if identity == nil { - return w.NewError("service identity is required") + return nil, w.NewError("service identity is required") } manager := resources.NewEnvironmentVariableManager() manager.SetIdentity(identity) @@ -2016,21 +2192,49 @@ func AppendRuntimeEnvironmentToFile( endpointMappings, resources.NetworkAccessFromRuntimeContext(runtimeContext), ); err != nil { - return w.Wrapf(err, "cannot add runtime endpoints") + return nil, w.Wrapf(err, "cannot add runtime endpoints") } environments, err := manager.All() if err != nil { - return w.Wrapf(err, "cannot get runtime environment variables") + return nil, w.Wrapf(err, "cannot get runtime environment variables") } - return appendEnvironmentVariablesToFile(ctx, filePath, environments) + return environments, nil } func appendEnvironmentVariablesToFile( ctx context.Context, filePath string, environments []*resources.EnvironmentVariable, +) error { + records, err := outputRecords(environments) + if err != nil { + return err + } + return writeOutputRecords(ctx, filePath, records) +} + +// writeOutputRecords is the one seam every `--output-env` write passes +// through, and therefore the one that enforces record integrity: a phase that +// preflights its complete batch and a caller that appends a single chunk reach +// the same check, so a direct internal caller cannot write a record the +// supported reader would refuse. It is a backstop, not the whole-batch +// guarantee — a caller that has already written a preceding chunk cannot be +// given no-write refusal by a check that runs this late, which is why the +// multi-chunk phases gather and validate everything first. +func writeOutputRecords( + ctx context.Context, + filePath string, + records []outputRecord, ) error { w := wool.Get(ctx).In("resources.AppendToFile", wool.Field("filePath", filePath)) + for index, record := range records { + if err := validOutputKey(record.key); err != nil { + return w.Wrapf(err, "cannot write output environment record %d", index) + } + if err := validOutputValue(record.line[len(record.key)+1:]); err != nil { + return w.Wrapf(err, "cannot write output environment carrier %s", record.key) + } + } filePath = strings.TrimSpace(filePath) if filePath == "" { return w.NewError("output environment file path is required") @@ -2052,6 +2256,9 @@ func appendEnvironmentVariablesToFile( if err != nil && !os.IsNotExist(err) { return w.Wrapf(err, "cannot read environment variables file") } + if err = validRetainedOutputRecords(string(existing)); err != nil { + return w.Wrap(err) + } var order []string values := map[string]string{} for _, line := range strings.Split(string(existing), "\n") { @@ -2064,11 +2271,13 @@ func appendEnvironmentVariablesToFile( } values[key] = line } - for _, env := range environments { - if _, seen := values[env.Key]; !seen { - order = append(order, env.Key) + for _, record := range records { + if _, seen := values[record.key]; !seen { + order = append(order, record.key) } - values[env.Key] = fmt.Sprintf("%s=%v", env.Key, env.Value) + // The line serialized when the batch was checked, never converted a + // second time here. + values[record.key] = record.line } var content strings.Builder for _, key := range order { diff --git a/pkg/orchestration/runner_accepted_network_mappings_test.go b/pkg/orchestration/runner_accepted_network_mappings_test.go index fcd884606..47fa66710 100644 --- a/pkg/orchestration/runner_accepted_network_mappings_test.go +++ b/pkg/orchestration/runner_accepted_network_mappings_test.go @@ -50,8 +50,13 @@ const ( realAgentUnknown = "unknown" // invents an endpoint that was never proposed realAgentDuplicate = "duplicate" // returns two native views for one endpoint realAgentInputs = "inputs" // echoes back the invocation inputs Init carried + realAgentRefuses = "refuses" // answers Init with ERROR and its own diagnostic ) +// realAgentRefusalMessage is the agent's own Init diagnostic, which the run +// must surface unchanged rather than replacing with a host-side explanation. +const realAgentRefusalMessage = "postmaster refused the data directory" + // exposedConfigurationName marks the runtime configuration the agent exports on // every Init. A rejected Init must not publish it. const exposedConfigurationName = "runtime" @@ -131,6 +136,11 @@ func proposedPorts(proposed []*basev0.NetworkMapping) map[uint16]bool { } func (agent *realAgent) Init(_ context.Context, req *runtimev0.InitRequest) (*runtimev0.InitResponse, error) { + if agent.mode == realAgentRefuses { + return &runtimev0.InitResponse{Status: &runtimev0.InitStatus{ + State: runtimev0.InitStatus_ERROR, Message: realAgentRefusalMessage, + }}, nil + } if image := req.GetRuntimeImage(); image != "" { parsed, err := reference.ParseNamed(image) if err != nil { diff --git a/pkg/orchestration/runner_output_env_test.go b/pkg/orchestration/runner_output_env_test.go index 01ef85d28..e42cc34ff 100644 --- a/pkg/orchestration/runner_output_env_test.go +++ b/pkg/orchestration/runner_output_env_test.go @@ -130,17 +130,9 @@ func TestAppendServiceProcessConfigurationsToFileIncludesSecretsAndFiltersDepend ) runtime := configuration("mind/mind", "runtime", "session_id", "session-123", false, native) - // The composition root's own structured document, handed to every service - // of the run as platform-core's merged fixture-composition is: it is - // carried under a key that hashes origin, name AND environment, so a - // writer that names no environment cannot write it at all. - document := &basev0.Configuration{Origin: resources.ConfigurationWorkspace, RuntimeContext: native, Infos: []*basev0.ConfigurationInformation{{ - Name: "fixture-composition", - Data: &basev0.ConfigurationData{Kind: "yaml", Content: []byte("schema: codefly.dev/authority/fixture-composition/v1\nroles: []\n")}, - }}} if err := AppendServiceProcessConfigurationsToFile( context.Background(), path, localRunEnvironment(), native, service, - []*basev0.Configuration{workspace, document}, + []*basev0.Configuration{workspace}, []*basev0.Configuration{postgres, containerOnly}, []*basev0.Configuration{runtime}, ); err != nil { @@ -156,21 +148,11 @@ func TestAppendServiceProcessConfigurationsToFileIncludesSecretsAndFiltersDepend "CODEFLY__WORKSPACE_CONFIGURATION__DOGFOOD__CAMPAIGN=headless\n", "CODEFLY__SERVICE_SECRET_CONFIGURATION__INFRA__POSTGRES__POSTGRES__READ_WRITE_CONNECTION=postgres://runtime\n", "CODEFLY__SERVICE_CONFIGURATION__MIND__MIND__RUNTIME__SESSION_ID=session-123\n", - // The document, under the key its reader derives for this environment. - resources.ConfigurationDocumentKey(resources.ConfigurationWorkspace, "fixture-composition", "local", false) + "=", } { if !strings.Contains(text, expected) { t.Fatalf("service process environment is missing %q; keys=%v", expected, environmentKeys(text)) } } - carrier := carrierValue(t, text, resources.ConfigurationDocumentKey(resources.ConfigurationWorkspace, "fixture-composition", "local", false)) - content, err := resources.DecodeConfigurationDocument(carrier, resources.ConfigurationWorkspace, "fixture-composition", "local", false) - if err != nil { - t.Fatalf("the written document does not read back for environment local: %v", err) - } - if !strings.Contains(string(content), "codefly.dev/authority/fixture-composition/v1") { - t.Fatalf("the document lost its content: %s", content) - } if strings.Contains(text, "REDIS") || strings.Contains(text, "redis://container") { t.Fatalf("container-only dependency leaked into native environment; keys=%v", environmentKeys(text)) } diff --git a/pkg/orchestration/runner_reload_addresses_test.go b/pkg/orchestration/runner_reload_addresses_test.go index 0ff761a61..5964a6038 100644 --- a/pkg/orchestration/runner_reload_addresses_test.go +++ b/pkg/orchestration/runner_reload_addresses_test.go @@ -73,11 +73,14 @@ func TestTheOutputEnvironmentFileKeepsOneValuePerKey(t *testing.T) { } require.NoError(t, appendEnvironmentVariablesToFile(ctx, path, environments)) } - write("CODEFLY__ENDPOINT__APP__API__REST__REST", "http://127.0.0.1:55634", "KEEP", "one") - write("CODEFLY__ENDPOINT__APP__API__REST__REST", "http://127.0.0.1:50695", "ADDED", "two") + // Every key is a real carrier spelling: the writer now holds the whole + // batch to the grammar the supported reader parses, so a placeholder like + // "KEEP" is refused rather than written (output_records.go). + write("CODEFLY__ENDPOINT__APP__API__REST__REST", "http://127.0.0.1:55634", "CODEFLY__KEPT", "one") + write("CODEFLY__ENDPOINT__APP__API__REST__REST", "http://127.0.0.1:50695", "CODEFLY__ADDED", "two") content, err := os.ReadFile(path) require.NoError(t, err) - require.Equal(t, "CODEFLY__ENDPOINT__APP__API__REST__REST=http://127.0.0.1:50695\nKEEP=one\nADDED=two\n", string(content), "one line per key, in first-seen order, with the latest value") + require.Equal(t, "CODEFLY__ENDPOINT__APP__API__REST__REST=http://127.0.0.1:50695\nCODEFLY__KEPT=one\nCODEFLY__ADDED=two\n", string(content), "one line per key, in first-seen order, with the latest value") info, err := os.Stat(path) require.NoError(t, err) require.Equal(t, os.FileMode(0o600), info.Mode().Perm()) From f26541ffe53c1f6442770e046440f2c09d5f4f71 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Wed, 28 Oct 2026 13:05:47 -0400 Subject: [PATCH 2/4] fix(configuration): compare the whole module selection across an artifact read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The post-read recheck compared the resolved directory and discarded the provenance the selection had already computed, so it could not see a selection that moved without its directory moving. ResolveModule answers from the overlay snapshot the workspace was loaded with. A non-managed selection — an explicit path or a worktree, which carries no receipt — therefore resolves to the same directory on both asks even after the overlay on disk has been re-pointed, and a selection can change mode or the request it answers while keeping its directory. Either way the path matched and the read was attributed to a selection this invocation no longer makes. SelectModule now returns a comparable descriptor carrying the existing owner evidence beside the directory: the disk-read directive, the receipt's request and what it produced, the derived mode, and the composed reference. The resolver holds it across the read and compares it whole. No new framework, no new source of truth, and every refusal is the one that already existed. Co-Authored-By: Claude Opus 5 --- pkg/composition/module_selection.go | 90 ++++++++++++++++++++--- pkg/orchestration/module_artifact.go | 29 +++++--- pkg/orchestration/module_artifact_test.go | 44 +++++++++++ 3 files changed, 141 insertions(+), 22 deletions(-) diff --git a/pkg/composition/module_selection.go b/pkg/composition/module_selection.go index 6fe0822b8..9a8cf4175 100644 --- a/pkg/composition/module_selection.go +++ b/pkg/composition/module_selection.go @@ -8,7 +8,53 @@ import ( "github.com/codefly-dev/core/resources" ) -// SelectedModuleDir is the directory a composed module resolves to right now, +// SelectedModule is one composed module's selection as this invocation holds +// it: where it resolves, how it was selected, and the request that selection +// answers. +// +// It is comparable with == on purpose, so a caller holding a selection across a +// read compares the WHOLE thing rather than picking a field. Its fields are the +// existing owner evidence — the resolution, the mode the materializer's own +// predicate derives, the machine-local directive as it stands on disk, the +// materialization receipt, and the reference the workspace makes — carried as +// scalars, because a pointer comparison answers "the same object" when the +// question is "the same selection". +type SelectedModule struct { + // Dir is the directory the module resolves to. + Dir string + // Verified is true only for a CLI-materialized selection whose mode is + // signature- or digest-checked. An explicit path or worktree is false: its + // bytes are local development evidence, not release authenticity. + Verified bool + // Mode is how the module is materialized, as a receipt records it. + Mode ResolutionMode + + // The machine-local directive, re-read from disk on every ask. This is what + // moves when a developer re-points a module between two reads — and it can + // move while the RESOLVED path does not, because ResolveModule answers from + // the overlay snapshot the workspace was loaded with. + DirectivePath string + DirectiveWorktree string + DirectiveGit bool + DirectivePinned bool + + // The materialization receipt: the request it answered and what that + // produced. Empty throughout for a selection the CLI did not materialize. + ReceiptSource string + ReceiptModule string + ReceiptRequested string + ReceiptVersion string + ReceiptPath string + ReceiptDigest string + ReceiptCommit string + + // The composed reference this selection answers. + RefSource string + RefModule string + RefVersion string +} + +// SelectModule is the selection of a composed module right now, // judged under the owner rules the materializer itself uses, and nothing else. // // It never pulls, materializes or reloads: a caller that reads bytes out of a @@ -26,7 +72,7 @@ import ( // its bytes are local development evidence rather than release authenticity. // Returning verified=false for those is the whole point — a caller that needs // to say where bytes came from must not call a worktree a release. -func SelectedModuleDir(ctx context.Context, workspace *resources.Workspace, module string) (dir string, verified bool, err error) { +func SelectModule(ctx context.Context, workspace *resources.Workspace, module string) (*SelectedModule, error) { var ref *resources.ModuleReference for _, candidate := range workspace.Modules { if candidate.Name == module { @@ -35,11 +81,11 @@ func SelectedModuleDir(ctx context.Context, workspace *resources.Workspace, modu } } if ref == nil { - return "", false, fmt.Errorf("module %s is not composed in workspace %s", module, workspace.Name) + return nil, fmt.Errorf("module %s is not composed in workspace %s", module, workspace.Name) } state, err := loadMaterializationState(ctx, workspace) if err != nil { - return "", false, err + return nil, err } var directive *resources.ModuleResolveDirective if state.overlay != nil { @@ -49,14 +95,36 @@ func SelectedModuleDir(ctx context.Context, workspace *resources.Workspace, modu mode := ResolutionModeFor(ref, directive, receipt, state.declared[module]) resolution, err := workspace.ResolveModule(ctx, ref) if err != nil { - return "", false, err + return nil, err } if resolution.Kind == resources.ResolutionPinned { - return "", false, fmt.Errorf("module %s is not materialized on this machine; resolve the composition before reading its tree", module) + return nil, fmt.Errorf("module %s is not materialized on this machine; resolve the composition before reading its tree", module) + } + selected := &SelectedModule{ + Dir: resolution.Dir, + Mode: mode, + RefSource: ref.Source, + RefModule: ref.Module, + RefVersion: ref.Version, + } + if directive != nil { + selected.DirectivePath = directive.Path + selected.DirectiveWorktree = directive.Worktree + selected.DirectiveGit = directive.Git + selected.DirectivePinned = directive.Pinned + } + if receipt != nil { + selected.ReceiptSource = receipt.Source + selected.ReceiptModule = receipt.Module + selected.ReceiptRequested = receipt.Requested + selected.ReceiptVersion = receipt.Version + selected.ReceiptPath = receipt.Path + selected.ReceiptDigest = receipt.Digest + selected.ReceiptCommit = receipt.Commit } if pinnedManaged(ref, directive, receipt.ResolvedPath(), state.roots.owned...) { if !moduleReceiptAnswers(ref, directive, receipt, mode) { - return "", false, fmt.Errorf("module %s materialization is unresolved or stale; resolve the composition before reading its tree", module) + return nil, fmt.Errorf("module %s materialization is unresolved or stale; resolve the composition before reading its tree", module) } // Two sources had to agree to get here, and they are read at different // moments: the overlay and the receipts come off disk NOW, while @@ -68,12 +136,12 @@ func SelectedModuleDir(ctx context.Context, workspace *resources.Workspace, modu // resolved directory to be the receipt's path, or under it where the // reference names a module subpath, and refuse the divergence rather // than pick a side. - if !sameSelectedDir(resolution.Dir, receipt.ResolvedPath()) { - return "", false, fmt.Errorf("module %s resolves to a different directory than its materialization receipt records; resolve the composition again before reading its tree", module) + if !sameSelectedDir(selected.Dir, receipt.ResolvedPath()) { + return nil, fmt.Errorf("module %s resolves to a different directory than its materialization receipt records; resolve the composition again before reading its tree", module) } - return resolution.Dir, !mode.Unverified(), nil + selected.Verified = !mode.Unverified() } - return resolution.Dir, false, nil + return selected, nil } // sameSelectedDir compares the resolved directory with the receipt's, and diff --git a/pkg/orchestration/module_artifact.go b/pkg/orchestration/module_artifact.go index 0004cd872..ed6c60ccb 100644 --- a/pkg/orchestration/module_artifact.go +++ b/pkg/orchestration/module_artifact.go @@ -51,10 +51,11 @@ func acquireModuleArtifact(ctx context.Context, workspace *resources.Workspace, if err := resources.ValidateModuleArtifactReference(ref); err != nil { return nil, err } - dir, _, err := composition.SelectedModuleDir(ctx, workspace, ref.Module) + selected, err := composition.SelectModule(ctx, workspace, ref.Module) if err != nil { return nil, err } + dir := selected.Dir root, err := os.OpenRoot(dir) if err != nil { return nil, fmt.Errorf("cannot confine module artifact reads to module %s", ref.Module) @@ -86,19 +87,25 @@ func acquireModuleArtifact(ctx context.Context, workspace *resources.Workspace, if !os.SameFile(before, after) || after.Size() != before.Size() || !after.ModTime().Equal(before.ModTime()) || int64(len(content)) != before.Size() { return nil, fmt.Errorf("module artifact %q in module %s changed while it was being read", ref.Path, ref.Module) } - // And the SELECTION, re-asked around the read. The same-open-file checks - // above only say the file did not move under us; they say nothing about - // whether this is still the file the composition selects. A - // re-materialization or an edited overlay between the open and here - // re-points the module, and these bytes would then be delivered as the - // selected module's while another directory is what the next load reads. - // Re-resolving is cheap (no pull, no reload) and its refusal is the same - // one as before the read. - reselected, _, err := composition.SelectedModuleDir(ctx, workspace, ref.Module) + // And the SELECTION, re-asked around the read — compared WHOLE, not by + // path. The same-open-file checks above only say the file did not move + // under us; they say nothing about whether this is still the selection the + // composition makes. + // + // The path alone cannot answer that. ResolveModule answers from the overlay + // snapshot the workspace was loaded with, so a non-managed selection — an + // explicit path or a worktree, which carries no receipt — resolves to the + // same directory on both asks even after the overlay on disk has been + // re-pointed; and a selection can change mode or the request it answers + // while keeping its directory. Comparing the descriptor, which carries the + // disk-read directive, the receipt's request and the derived mode beside + // the directory, is what makes those visible. It is cheap: no pull, no + // reload, and the refusal is the same one as before the read. + reselected, err := composition.SelectModule(ctx, workspace, ref.Module) if err != nil { return nil, err } - if reselected != dir { + if *reselected != *selected { return nil, fmt.Errorf("module %s was reselected while artifact %q was being read; resolve the composition again", ref.Module, ref.Path) } return content, nil diff --git a/pkg/orchestration/module_artifact_test.go b/pkg/orchestration/module_artifact_test.go index 18d8ff973..9056303cf 100644 --- a/pkg/orchestration/module_artifact_test.go +++ b/pkg/orchestration/module_artifact_test.go @@ -10,6 +10,7 @@ import ( "syscall" "testing" + "github.com/codefly-dev/cli/pkg/composition" basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" "github.com/codefly-dev/core/resources" "github.com/stretchr/testify/require" @@ -185,3 +186,46 @@ func TestTheRetainedDeliveryIsDetached(t *testing.T) { require.Equal(t, body, detached[0].Infos[0].ConfigurationValues[0].Value, "the retained delivery is what the service received, whatever happens to the slices it came from") } + +// TestTheSelectionIsComparedWholeNotByPath is the case a path-only recheck +// cannot see. +// +// A non-managed selection — an explicit overlay path, carrying no receipt — +// resolves through the overlay snapshot the workspace was LOADED with, so it +// answers the same directory on every ask even after the overlay on disk has +// been re-pointed. Comparing directories across the read therefore passes +// while the selection has actually moved. The descriptor carries the disk-read +// directive beside the resolved directory, so the move is visible. +func TestTheSelectionIsComparedWholeNotByPath(t *testing.T) { + ctx := context.Background() + workspace := writeTempWorkspace(t, map[string]string{ + "workspace.codefly.yaml": `name: artifact-reselect +layout: modules +modules: + - name: wiki + path: modules/wiki +`, + "modules/wiki/module.codefly.yaml": excludedRootExportModule, + "modules/wiki/services/backend/service.codefly.yaml": excludedRootExportService, + "modules/wiki/docs/install.md": "A\n", + "codefly.local.yaml": "resolve:\n wiki:\n path: modules/wiki\n", + }) + + first, err := composition.SelectModule(ctx, workspace, "wiki") + require.NoError(t, err) + require.False(t, first.Verified, "an explicit overlay path is local development evidence") + require.Equal(t, "modules/wiki", first.DirectivePath) + + // Re-point the module on disk, to a directory that resolves the same way + // through the loaded snapshot. + require.NoError(t, os.MkdirAll(filepath.Join(workspace.Dir(), "elsewhere/wiki/docs"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(workspace.Dir(), "codefly.local.yaml"), + []byte("resolve:\n wiki:\n path: elsewhere/wiki\n"), 0o600)) + + second, err := composition.SelectModule(ctx, workspace, "wiki") + require.NoError(t, err) + require.NotEqual(t, *first, *second, "the selection moved, and the descriptor says so") + require.Equal(t, "elsewhere/wiki", second.DirectivePath, "which the disk-read directive is what carries") + require.Equal(t, first.Dir, second.Dir, + "while the RESOLVED directory is unchanged — which is exactly why a path-only recheck passed") +} From ac87426bc9b86b9803b95215dbc76f6039fe7de7 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Wed, 28 Oct 2026 13:09:35 -0400 Subject: [PATCH 3/4] fix(run): keep Start's own gathering preconditions reporting themselves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retained-delivery guard sat ahead of the output gathering, so a runner with no consumer module — which must refuse rather than write dependency addresses whose visibility nothing can check — reported the missing delivery instead of the missing module, and TestOutputEnvRefusesToWriteWithoutAConsumerModule went red on CI. The guard now sits immediately before the validation it protects. The gathering only reads and converts, writing nothing, so no no-write guarantee moves. Co-Authored-By: Claude Opus 5 --- pkg/orchestration/runner.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/orchestration/runner.go b/pkg/orchestration/runner.go index 64f2652b0..8fd5d2ad3 100644 --- a/pkg/orchestration/runner.go +++ b/pkg/orchestration/runner.go @@ -1082,16 +1082,6 @@ func (runner *Runner) Start(ctx context.Context) (*OutputProperty, error) { } } - // A nil baseline is not "nothing to protect": Core reads it as an empty - // set of bound destinations and passes, so a Start that never had a - // successful Init would check this phase's offers against nothing. The - // missing evidence is an orchestration error, caught here rather than - // converted into an empty comparison — and after BeginRuntimeStart, so the - // attempt still completes as not-started. - if runner.delivered == nil { - return nil, w.NewError("cannot start %s: no successful initialization retained its configuration delivery, so this phase's configuration offers and overrides cannot be checked against what the service was admitted with", runner.instance.Unique()) - } - // Build the request. Everything this phase will deliver is gathered and // checked BEFORE the running instance is stopped and before either output // append: a refusal then leaves an absent output absent and an existing @@ -1111,6 +1101,20 @@ func (runner *Runner) Start(ctx context.Context) (*OutputProperty, error) { return nil, err } + // A nil baseline is not "nothing to protect": Core reads it as an empty set + // of bound destinations and passes, so a Start that never had a successful + // Init would check this phase's offers against nothing. The missing + // evidence is an orchestration error. + // + // It sits immediately before the validation it protects, not ahead of the + // gathering above: the gathering only reads and converts, writing nothing, + // and its own preconditions — a consumer module to narrow dependency + // visibility against, most of all — must keep reporting themselves rather + // than being masked by this one. + if runner.delivered == nil { + return nil, w.NewError("cannot start %s: no successful initialization retained its configuration delivery, so this phase's configuration offers and overrides cannot be checked against what the service was admitted with", runner.instance.Unique()) + } + // The retained Init payload is the comparison baseline, not a re-emission: // Start sends no fresh configuration in its request and exports only the // dependency batch, so a dependency offer that aliases a destination a From c04a79a7ae03e150c9faa8b48e81bd23546cddc0 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Wed, 28 Oct 2026 13:29:37 -0400 Subject: [PATCH 4/4] fix(configuration): carry the overlay origin in the module selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A directive path is relative to the overlay file that declares it, so a parent overlay and a nearer workspace overlay saying the same `path: modules/wiki` select different directories. The descriptor compared the raw path text, which is identical in both, and the resolved directory, which is also identical because ResolveModule answers from the snapshot the workspace was loaded with — so a nearer overlay appearing mid-read moved the selection and compared equal. The selection now carries the directory its overlay and resolution record were read from, which is NearestOverlayDir's existing answer and already computed by loadMaterializationState. No new framework and no new source of truth. Closes the last case from the independent selection-provenance probe; its TestIndependentOverlayOriginChange and TestIndependentSamePathReceiptChange both pass, and the behaviour has its own regression here. Co-Authored-By: Claude Opus 5 --- pkg/composition/module_selection.go | 12 +++++++ pkg/orchestration/module_artifact_test.go | 42 +++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/pkg/composition/module_selection.go b/pkg/composition/module_selection.go index 9a8cf4175..f414f343a 100644 --- a/pkg/composition/module_selection.go +++ b/pkg/composition/module_selection.go @@ -29,6 +29,17 @@ type SelectedModule struct { // Mode is how the module is materialized, as a receipt records it. Mode ResolutionMode + // OverlayDir is the directory whose overlay and resolution record this + // selection was read from — NearestOverlayDir's answer, or the workspace + // when nothing is nearer. + // + // It is part of the selection because a directive path is relative to the + // overlay file that declares it. A nearer overlay appearing with the SAME + // path text re-points the module without changing one character of the + // directive, so the raw path cannot tell the two selections apart and this + // is what does. + OverlayDir string + // The machine-local directive, re-read from disk on every ask. This is what // moves when a developer re-points a module between two reads — and it can // move while the RESOLVED path does not, because ResolveModule answers from @@ -103,6 +114,7 @@ func SelectModule(ctx context.Context, workspace *resources.Workspace, module st selected := &SelectedModule{ Dir: resolution.Dir, Mode: mode, + OverlayDir: state.recordDir, RefSource: ref.Source, RefModule: ref.Module, RefVersion: ref.Version, diff --git a/pkg/orchestration/module_artifact_test.go b/pkg/orchestration/module_artifact_test.go index 9056303cf..dcf9505f4 100644 --- a/pkg/orchestration/module_artifact_test.go +++ b/pkg/orchestration/module_artifact_test.go @@ -229,3 +229,45 @@ modules: require.Equal(t, first.Dir, second.Dir, "while the RESOLVED directory is unchanged — which is exactly why a path-only recheck passed") } + +// TestANearerOverlayChangesTheSelectionWithoutChangingItsPath is the last way +// a selection can move while every other field of it holds still. +// +// A directive path is relative to the overlay file that declares it. A parent +// overlay saying `path: modules/wiki` and a nearer workspace overlay saying the +// very same thing select DIFFERENT directories, so comparing the raw path text +// cannot tell them apart — and the resolved directory cannot either, because +// ResolveModule answers from the snapshot the workspace was loaded with. The +// overlay the selection was read from is what distinguishes them. +func TestANearerOverlayChangesTheSelectionWithoutChangingItsPath(t *testing.T) { + ctx := context.Background() + parent := t.TempDir() + child := filepath.Join(parent, "composition") + write := func(name, body string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(name), 0o755)) + require.NoError(t, os.WriteFile(name, []byte(body), 0o600)) + } + const overlay = "resolve:\n wiki:\n path: modules/wiki\n" + write(filepath.Join(parent, "codefly.local.yaml"), overlay) + write(filepath.Join(child, "workspace.codefly.yaml"), + "name: origin-change\nlayout: modules\nmodules:\n - name: wiki\n source: github.com/example/wiki\n version: 1.0.0\n") + write(filepath.Join(parent, "modules/wiki/docs/install.md"), "A\n") + write(filepath.Join(child, "modules/wiki/docs/install.md"), "B\n") + + workspace, err := resources.LoadWorkspaceFromDir(ctx, child) + require.NoError(t, err) + before, err := composition.SelectModule(ctx, workspace, "wiki") + require.NoError(t, err) + require.Equal(t, parent, before.OverlayDir, "the parent overlay is the nearest one") + + // The same text, declared nearer. Nothing about the directive changes. + write(filepath.Join(child, "codefly.local.yaml"), overlay) + after, err := composition.SelectModule(ctx, workspace, "wiki") + require.NoError(t, err) + + require.Equal(t, before.DirectivePath, after.DirectivePath, "the path text is identical") + require.Equal(t, before.Dir, after.Dir, "and so is the resolved directory, from the loaded snapshot") + require.NotEqual(t, *before, *after, "yet the selection moved, and the overlay origin says so") + require.Equal(t, child, after.OverlayDir) +}