Skip porter-state for modifies: false custom actions

pkg/cnab/provider/action.go

@@ -270,9 +270,22 @@ func (r *Runtime) SaveOperationResult(ctx context.Context, opResult driver.Opera
 		bigerr = multierror.Append(bigerr, fmt.Errorf("error updating installation record for %s\n%#v: %w", installation, installation, err))
 	}
 
+	extBun := cnab.ExtendedBundle{Bundle: run.Bundle}
+	actionModifies := true
+	if action, err := run.Bundle.GetAction(run.Action); err == nil {
+		actionModifies = action.Modifies
+	}
+
 	for outputName, outputValue := range opResult.Outputs {
+		// porter-state tracks bundle-managed resource state. Skip persisting it
+		// for modifies:false actions so that a read-only invoke cannot overwrite
+		// the installation's state record.
+		if !actionModifies && extBun.IsInternalOutput(outputName) {
+			continue
+		}
+
 		output := result.NewOutput(outputName, []byte(outputValue))
-		output, err = r.sanitizer.CleanOutput(ctx, output, cnab.ExtendedBundle{Bundle: run.Bundle})
+		output, err = r.sanitizer.CleanOutput(ctx, output, extBun)
 		if err != nil {
 			bigerr = multierror.Append(bigerr, fmt.Errorf("error sanitizing sensitive %s output for %s run of installation %s\n%#v: %w", output.Name, run.Action, installation, output, err))
 		}

pkg/cnab/provider/action_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/cnabio/cnab-go/bundle"
+	"github.com/cnabio/cnab-go/bundle/definition"
 	"github.com/cnabio/cnab-go/driver"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -73,3 +74,93 @@ func TestAddFiles(t *testing.T) {
 	require.Contains(t, op.Files, config.ClaimFilepath, "The claim should have been injected into the bundle")
 	test.CompareGoldenFile(t, "testdata/want-claim.json", op.Files[config.ClaimFilepath])
 }
+
+func TestSaveOperationResult_ModifiesFalse_SkipsPorterState(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	d := NewTestRuntime(t)
+	defer d.Close()
+
+	instName := "mybuns"
+	bun := bundle.Bundle{
+		Actions: map[string]bundle.Action{
+			"dry-run": {Modifies: false},
+		},
+		Outputs: map[string]bundle.Output{
+			"porter-state": {Definition: "porter-state", Path: "/cnab/app/outputs/porter-state.tgz"},
+			"user-output":  {Definition: "user-output", Path: "/cnab/app/outputs/user-output"},
+		},
+		Definitions: map[string]*definition.Schema{
+			"porter-state": {Comment: cnab.PorterInternal},
+			"user-output":  {Type: "string"},
+		},
+	}
+	i := d.TestInstallations.CreateInstallation(storage.NewInstallation("", instName), d.TestInstallations.SetMutableInstallationValues)
+	run := storage.NewRun("", instName)
+	run.Bundle = bun
+	run.Action = "dry-run"
+	run = d.TestInstallations.CreateRun(run, d.TestInstallations.SetMutableRunValues)
+	result := run.NewResult(cnab.StatusSucceeded)
+
+	opResult := driver.OperationResult{
+		Outputs: map[string]string{
+			"porter-state": "state-data",
+			"user-output":  "hello",
+		},
+	}
+
+	err := d.SaveOperationResult(ctx, opResult, i, run, result)
+	require.NoError(t, err)
+
+	outputs, err := d.TestInstallations.GetOutputs(ctx, run.ID)
+	require.NoError(t, err)
+
+	_, hasPorterState := outputs.GetByName("porter-state")
+	assert.False(t, hasPorterState, "porter-state should not be saved for modifies:false actions")
+
+	_, hasUserOutput := outputs.GetByName("user-output")
+	assert.True(t, hasUserOutput, "user-defined outputs should still be saved")
+}
+
+func TestSaveOperationResult_ModifiesTrue_SavesPorterState(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	d := NewTestRuntime(t)
+	defer d.Close()
+
+	instName := "mybuns"
+	bun := bundle.Bundle{
+		Actions: map[string]bundle.Action{
+			"rotate-creds": {Modifies: true},
+		},
+		Outputs: map[string]bundle.Output{
+			"porter-state": {Definition: "porter-state", Path: "/cnab/app/outputs/porter-state.tgz"},
+		},
+		Definitions: map[string]*definition.Schema{
+			"porter-state": {Comment: cnab.PorterInternal},
+		},
+	}
+	i := d.TestInstallations.CreateInstallation(storage.NewInstallation("", instName), d.TestInstallations.SetMutableInstallationValues)
+	run := storage.NewRun("", instName)
+	run.Bundle = bun
+	run.Action = "rotate-creds"
+	run = d.TestInstallations.CreateRun(run, d.TestInstallations.SetMutableRunValues)
+	result := run.NewResult(cnab.StatusSucceeded)
+
+	opResult := driver.OperationResult{
+		Outputs: map[string]string{
+			"porter-state": "state-data",
+		},
+	}
+
+	err := d.SaveOperationResult(ctx, opResult, i, run, result)
+	require.NoError(t, err)
+
+	outputs, err := d.TestInstallations.GetOutputs(ctx, run.ID)
+	require.NoError(t, err)
+
+	_, hasPorterState := outputs.GetByName("porter-state")
+	assert.True(t, hasPorterState, "porter-state should be saved for modifies:true actions")
+}

pkg/runtime/runtime_manifest_test.go

@@ -1404,6 +1404,60 @@ func createTestStateArchive(t *testing.T, stateName, content string) []byte {
 	return buf.Bytes()
 }
 
+func TestFinalize_PackStateBag(t *testing.T) {
+	mContent := `schemaVersion: 1.0.0-alpha.2
+state:
+- name: mystate
+  path: /path/to/mystate
+
+install:
+- mymixin:
+    Parameters:
+      Thing: test
+`
+	setup := func(t *testing.T, action string, modifies bool, isBuiltin bool) (*RuntimeManifest, *config.TestConfig) {
+		testConfig := config.NewTestConfig(t)
+		rm := runtimeManifestFromStepYaml(t, testConfig, mContent)
+		rm.Action = action
+
+		actions := map[string]bundle.Action{}
+		if !isBuiltin {
+			actions[action] = bundle.Action{Modifies: modifies}
+		}
+		rm.bundle = cnab.NewBundle(bundle.Bundle{Actions: actions})
+
+		require.NoError(t, testConfig.FileSystem.MkdirAll(config.BundleOutputsDir, pkg.FileModeDirectory))
+		return rm, testConfig
+	}
+
+	t.Run("modifies false still writes porter-state", func(t *testing.T) {
+		// packStateBag always runs so the CNAB driver can collect the output.
+		// Skipping the save on the host side is what prevents state from being
+		// persisted for modifies:false actions.
+		rm, testConfig := setup(t, "dry-run", false, false)
+		require.NoError(t, rm.Finalize(context.Background()))
+		exists, err := testConfig.FileSystem.Exists("/cnab/app/outputs/porter-state")
+		require.NoError(t, err)
+		assert.True(t, exists, "porter-state must be written so the CNAB driver can collect it")
+	})
+
+	t.Run("modifies true writes porter-state", func(t *testing.T) {
+		rm, testConfig := setup(t, "rotate-creds", true, false)
+		require.NoError(t, rm.Finalize(context.Background()))
+		exists, err := testConfig.FileSystem.Exists("/cnab/app/outputs/porter-state")
+		require.NoError(t, err)
+		assert.True(t, exists, "porter-state should be written for modifies: true actions")
+	})
+
+	t.Run("built-in action writes porter-state", func(t *testing.T) {
+		rm, testConfig := setup(t, cnab.ActionInstall, false, true)
+		require.NoError(t, rm.Finalize(context.Background()))
+		exists, err := testConfig.FileSystem.Exists("/cnab/app/outputs/porter-state")
+		require.NoError(t, err)
+		assert.True(t, exists, "porter-state should be written for built-in actions")
+	})
+}
+
 func TestResolveInvocationImage(t *testing.T) {
 	testcases := []struct {
 		name                string

pkg/storage/run_test.go

@@ -107,6 +107,27 @@ func TestRun_ShouldRecord(t *testing.T) {
 		assert.True(t, r.ShouldRecord())
 	})
 
+	t.Run("modifies false, stateful, with applyTo output", func(t *testing.T) {
+		// Stateful + modifies:false + user output: run and outputs are recorded.
+		// porter-state isolation is enforced separately in Finalize(), not here.
+		b := bundle.Bundle{
+			Actions: map[string]bundle.Action{
+				"dry-run": {
+					Modifies:  false,
+					Stateless: false,
+				},
+			},
+			Outputs: map[string]bundle.Output{
+				"diff": {
+					ApplyTo: []string{"dry-run"},
+				},
+			},
+		}
+
+		r := Run{Bundle: b, Action: "dry-run"}
+		assert.True(t, r.ShouldRecord())
+	})
+
 	t.Run("has only internal bundle level output", func(t *testing.T) {
 		b := bundle.Bundle{
 			Definitions: definition.Definitions{

tests/integration/invoke_test.go

@@ -62,3 +62,51 @@ func TestInvokeCustomAction(t *testing.T) {
 	require.NoError(t, err, "GetLastClaim failed")
 	assert.Equal(t, "zombies", c.Action, "the custom action wasn't recorded in the installation")
 }
+
+func TestInvoke_ModifiesFalse_DoesNotPersistState(t *testing.T) {
+	t.Parallel()
+
+	p := porter.NewTestPorter(t)
+	defer p.Close()
+	ctx := p.SetupIntegrationTest()
+
+	err := p.Create()
+	require.NoError(t, err)
+
+	bundleName := p.AddTestBundleDir("testdata/bundles/bundle-with-modifies-false-action", true)
+
+	// Install to establish initial porter-state
+	installOpts := porter.NewInstallOptions()
+	err = installOpts.Validate(ctx, []string{}, p.Porter)
+	require.NoError(t, err)
+	err = p.InstallBundle(ctx, installOpts)
+	require.NoError(t, err, "install should have succeeded")
+
+	// Capture the porter-state output written during install
+	stateAfterInstall, err := p.Installations.GetLastOutput(ctx, "", bundleName, "porter-state")
+	require.NoError(t, err, "porter-state output should exist after install")
+
+	// Invoke the modifies:false dry-run action
+	invokeOpts := porter.NewInvokeOptions()
+	invokeOpts.Action = "dry-run"
+	err = invokeOpts.Validate(ctx, []string{}, p.Porter)
+	require.NoError(t, err)
+	err = p.InvokeBundle(ctx, invokeOpts)
+	require.NoError(t, err, "dry-run invoke should have succeeded")
+
+	// porter-state must not have been updated by the dry-run
+	stateAfterDryRun, err := p.Installations.GetLastOutput(ctx, "", bundleName, "porter-state")
+	require.NoError(t, err, "porter-state output should still exist after dry-run")
+	assert.Equal(t, stateAfterInstall.ResultID, stateAfterDryRun.ResultID,
+		"porter-state should not have been updated by a modifies:false action")
+
+	// The dry-run run record must exist
+	lastRun, err := p.Installations.GetLastRun(ctx, "", bundleName)
+	require.NoError(t, err, "GetLastRun failed")
+	assert.Equal(t, "dry-run", lastRun.Action, "dry-run should have been recorded")
+
+	// The user-defined output from dry-run must have been saved
+	dryRunOutput, err := p.Installations.GetOutput(ctx, lastRun.ID, "dry-run-result")
+	require.NoError(t, err, "dry-run-result output should have been saved")
+	assert.Contains(t, string(dryRunOutput.Value), "dry-run-ok")
+}

tests/integration/testdata/bundles/bundle-with-modifies-false-action/porter.yaml

@@ -0,0 +1,59 @@
+schemaVersion: 1.0.0-alpha.1
+name: bundle-with-modifies-false-action
+version: 0.1.0
+description: "Bundle for testing that modifies:false actions do not persist porter-state"
+registry: "localhost:5000"
+
+mixins:
+  - exec
+
+state:
+  - name: some_state
+    path: /path/to/some_state
+
+customActions:
+  dry-run:
+    description: "Dry run - does not modify resources"
+    stateless: false
+    modifies: false
+
+install:
+  - exec:
+      description: "Install"
+      command: sh
+      arguments:
+        - -c
+        - |
+          mkdir -p /path/to
+          echo "installed" > /path/to/some_state
+
+dry-run:
+  - exec:
+      description: "Dry run"
+      command: sh
+      arguments:
+        - -c
+        - echo "dry-run-ok" > /cnab/app/outputs/dry-run-result
+
+upgrade:
+  - exec:
+      description: "Upgrade"
+      command: sh
+      arguments:
+        - -c
+        - echo "upgraded"
+
+uninstall:
+  - exec:
+      description: "Uninstall"
+      command: sh
+      arguments:
+        - -c
+        - echo "uninstalled"
+
+outputs:
+  - name: dry-run-result
+    type: string
+    path: /cnab/app/outputs/dry-run-result
+    applyTo:
+      - dry-run