Skip porter-state for modifies: false custom actions

[kichristensen]

What does this change

Custom actions declared with `modifies: false` (e.g. `dry-run`) would silently overwrite the installation's `porter-state` whenever they produced outputs. This happened because `SaveOperationResult()` persisted all outputs unconditionally, regardless of the action's `modifies` flag.

This change adds a guard in `SaveOperationResult()`: if the action has `Modifies: false`, internal outputs such as `porter-state` are skipped during persistence. `Finalize()` still packs the state bag unconditionally so the runtime output file is produced, but the persistence layer discards it for non-modifying actions. For built-in actions (`install`, `upgrade`, `uninstall`) and `modifies: true` actions the existing behavior is preserved.

Run records and user-defined outputs for `modifies: false` actions continue to be persisted — this is correct per the Porter docs, which state that outputs are recorded when they explicitly apply to an action via `applyTo`.

What issue does it fix

Closes #3579

Notes for the reviewer

• No changes to `ShouldRecord()` — its `hasOutput` OR-term is intentional. The Porter docs confirm that outputs are recorded when they explicitly apply to an action via `applyTo`. State isolation is a separate concern enforced in `SaveOperationResult()`.
• The guard uses `Run.ActionModifies()` and `ExtendedBundle.IsInternalOutput()` to identify outputs that must not be persisted for non-modifying actions.

Checklist

• Did you write tests?
• Did you write documentation?
• Did you change porter.yaml or a storage document record? Update the corresponding schema file.
• If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

[schristoff]

I feel like we should be modifying "ShouldRecord()" to execute properly in this area by having a bool checked here, what do you think?

[kichristensen]

I feel like we should be modifying "ShouldRecord()" to execute properly in this area by having a bool checked here, what do you think?

@schristoff If I understand you correctly. I don't think there is anything wrong in ShouldRecord(), the documentation explicitly states that modifies: false can still record a run and the outputs. But state shouldn't be updated when a action has modifies: false.
What do you think?

[schristoff]

Yeah, I agree with that. I believe we should be leveraging that bool more to disregard writing the state, if that makes sense.

[Copilot]

Pull request overview

This PR prevents modifies:false custom actions from overwriting persisted Porter state while still allowing their run records and user-defined outputs to be saved.

Changes:

• Adds Run.ActionModifies() and reuses it in ShouldRecord().
• Filters internal outputs such as porter-state during SaveOperationResult() for non-modifying actions.
• Adds unit and integration coverage plus a test bundle for the modifies:false state-isolation scenario.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/cnab/provider/action.go	Skips persisting internal outputs for non-modifying actions.
pkg/cnab/provider/action_test.go	Adds tests for saving/skipping porter-state based on action modification behavior.
pkg/storage/run.go	Adds reusable action modification helper and applies it to run recording logic.
pkg/storage/run_test.go	Adds tests for ActionModifies() and output-driven run recording.
pkg/runtime/runtime_manifest_test.go	Adds tests documenting that finalization still writes the state output file.
tests/integration/invoke_test.go	Adds an end-to-end regression test for modifies:false state persistence.
tests/integration/testdata/bundles/bundle-with-modifies-false-action/porter.yaml	Adds a fixture bundle with state, a modifies:false action, and an action-scoped output.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.