fix: skip porter-state on host for modifies:false

The previous fix skipped packStateBag() inside the invocation
image, but the CNAB driver requires all outputs with no default
to be present — so it failed with "required output porter-state
is missing and has no default".

Move the guard to SaveOperationResult on the host side: when the
action has modifies:false, skip persisting internal outputs
(porter-state) to the installation's output record while still
allowing packStateBag() to write the file for the driver.

Signed-off-by: Kim Christensen <kimworking@gmail.com>

Author: kichristensen

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.go

@@ -760,14 +760,9 @@ func (m *RuntimeManifest) Finalize(ctx context.Context) error {
 		bigErr = multierror.Append(bigErr, err)
 	}
 
-	// Skip state persistence for modifies: false actions. Such actions declare
-	// they do not change bundle-managed resources, so writing porter-state back
-	// would contradict that contract.
-	if action, err := m.bundle.GetAction(m.Action); err != nil || action.Modifies {
-		// Always try to persist state, even when errors occur
-		if err := m.packStateBag(ctx); err != nil {
-			bigErr = multierror.Append(bigErr, err)
-		}
+	// Always try to persist state, even when errors occur
+	if err := m.packStateBag(ctx); err != nil {
+		bigErr = multierror.Append(bigErr, err)
 	}
 
 	return bigErr.ErrorOrNil()

pkg/runtime/runtime_manifest_test.go

@@ -1430,12 +1430,15 @@ install:
 		return rm, testConfig
 	}
 
-	t.Run("modifies false skips porter-state", func(t *testing.T) {
+	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.False(t, exists, "porter-state should not be written for modifies: false actions")
+		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) {