fix: apply invocation config to workflow Data after callback

[danskmt]

User description

Description

Workflows can return Data created with workflow.NewData without WithConfiguration, so IN_MEMORY_THRESHOLD_BYTES and TEMP_DIR_PATH were not applied to those payloads.

After each workflow callback returns, EngineImpl.Invoke calls applyConfiguration on each *DataImpl in the output slice so in-memory payloads can spill to the configured temp directory when above threshold. applyConfiguration is package-private on DataImpl (same package as the engine) so it is not exported from the workflow package.

Adds tests in dataimpl_test.go for spill and no-op cases, and in engine_test.go for behavior after Invoke.

Checklist

• Tests added and all succeed (make test)
• Regenerated mocks, etc. (make generate)
• Linted (make lint)
• Test your changes work for the CLI
  1. Clone / pull the latest CLI main.
  2. Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
     • Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
  3. Run go mod tidy in the cliv2 directory.
  4. Run the CLI tests and do any required manual testing.
  5. Open a PR in the CLI repo now with the go.mod and go.sum changes.
  • Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.

PR in CLI snyk/cli#6792

Where reviewers should start

• pkg/workflow/dataimpl.go — applyConfiguration
• pkg/workflow/engineimpl.go — post-process loop after callback

Risk assessment

Low. Default production behavior is unchanged unless spill-related settings are enabled; when they are, returned Data placement aligns with engine configuration instead of leaving eligible payloads in memory.

---

PR Type

Bug fix, Enhancement, Tests

---

Description

• Apply engine configuration to workflow output Data.

• Enable payload spilling based on engine settings.

• Introduce DataImpl.applyConfiguration method logic.

• Add comprehensive tests for configuration application.

---

Diagram Walkthrough

flowchart LR
  engine_invoke["EngineImpl.Invoke"] --> workflow_callback["Workflow Callback"];
  workflow_callback --> output_data["Output Data"];
  output_data --> post_processing_loop["Apply Config Loop"];
  post_processing_loop --> dataimpl_apply_config["DataImpl.applyConfiguration"];
  dataimpl_apply_config --> spill_to_disk["Payload Spills to Disk (if needed)"];

Loading

File Walkthrough

	Relevant files
Bug fix	dataimpl.go Implement DataImpl configuration application logic              pkg/workflow/dataimpl.go Modified WithConfiguration to conditionally apply threshold and temp dir values. Introduced applyConfiguration method to relocate in-memory payloads to disk based on configuration. +32/-2
Tests	dataimpl_test.go Add tests for DataImpl applyConfiguration                                pkg/workflow/dataimpl_test.go Added several new test cases for applyConfiguration. Tests cover spill behavior, no-op scenarios, default values, and missing temp directory paths. +135/-0  engine_test.go Test engine Invoke output configuration application            pkg/workflow/engine_test.go Added a new test Test_Invoke_AppliesConfigurationToOutput. Verifies that engine's invocation applies configuration to output data, causing spills. +39/-0
Enhancement	engineimpl.go Apply engine configuration to Invoke output                            pkg/workflow/engineimpl.go Modified Invoke to iterate over workflow output Data. Calls di.applyConfiguration(options.config) on each DataImpl to apply engine settings. +9/-0

---

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ No major issues detected

📚 Repository Context Analyzed This review considered 12 relevant code sections from 8 files (average relevance: 0.95) pkg/mocks/workflow.go (3 segments, lines 1-238) pkg/workflow/enginewrapper.go (lines 1-125) pkg/workflow/types.go (lines 1-115) pkg/local_workflows/output_workflow/other_tools.go (lines 1-144) pkg/app/app.go (lines 235-386) pkg/workflow/dataimpl.go (2 segments, lines 1-273) pkg/workflow/engineimpl.go (2 segments, lines 279-409) ... and 1 more file(s)

[robertolopezlopez]

in an ideal world (at least the one in my head) there should be a receiver func (d *DataImpl) WithConfiguration(...). As this may be problematic to implement now (too many usages off the task scope), I suggest the following:

func WithConfiguration(config configuration.Configuration) Option {
   return func(d *DataImpl) { d.configureFrom(config) }
}

func (d *DataImpl) configureFrom(config configuration.Configuration) {
   // copy in memory threshold and temp dir path if present in config
}

func (d *DataImpl) applyConfiguration(config configuration.Configuration) {
   ... your nil check
   d.configureFrom(config)
   ... rest of logic from your method
}

what do you think - this doesn't change the existing signatures which is probably used from other places, while keeping the state in the receiver

[robertolopezlopez]

/describe

[snyk-pr-review-bot]

PR Description updated to latest commit (86de328)

[basti-snyk]

This PR does not make sense to me. Which problem are we solving?

[basti-snyk]

Should-Fix (correctness): new nil-config panic path. Invoke now unconditionally calls applyConfiguration(options.config) for every *DataImpl output, and this first line dereferences the config. When a caller passes a nil config — Invoke(id, WithConfig(nil)) or the deprecated InvokeWithConfig(id, nil) — options.config is nil, so this panics (nil interface method call). Before this PR that path was harmless: the engine only stored the config (newInvocationContext, SetConfiguration) and never dereferenced it. Root-cause fix is one line at the top of this method: if config == nil { return }. Please also add a test: Invoke with WithConfig(nil) and a workflow returning NewData(...) must not panic. — AI review

[basti-snyk]

Should-Fix (contract): engine silently overrides a workflow's explicit WithConfiguration. This loop applies the engine config to every returned *DataImpl, and applyConfiguration re-runs WithConfiguration, overwriting inMemoryThreshold/tempDirPath. A workflow that deliberately built its Data with NewData(..., WithConfiguration(customCfg)) (e.g. a higher threshold to keep a payload in memory on purpose) has that intent silently replaced by the engine defaults and force-spilled. applyConfiguration can't distinguish "never configured" (the case this PR targets) from "deliberately configured". Either apply only when the Data was never configured, or document on Invoke that engine config always wins last. This override path is also untested (new tests only cover Data created without WithConfiguration). — AI review

[basti-snyk]

Suggestion: relocation runs even when the callback returned an error. This loop is outside any if err == nil guard, so when a workflow returns partial output alongside a non-nil err, that soon-to-be-discarded output is still spilled to disk, leaving orphan temp files for data nobody consumes. Guarding the loop with if err == nil avoids the wasted I/O. — AI review

[basti-snyk]

Suggestion: exported-API semantic change worth a direct-caller test. WithConfiguration previously set inMemoryThreshold/tempDirPath unconditionally (unset key → 0/""); it now skips a key when config.Get(...) == nil. This is a genuine fix (an unset threshold no longer means "spill everything"), but WithConfiguration is public and no test asserts the new behavior for direct external callers when one key is set and the other is unset. Any caller that relied on "unset threshold ⇒ 0 ⇒ spill" changes silently. Add a focused unit test for the mixed set/unset case. — AI review

[basti-snyk]

ℹ️ Automated multi-agent review summary — non-blocking. Detailed findings are posted as inline comments.

PR #606 — apply invocation config to output Data after callback

Overall: solid, focused fix with good test coverage for the intended path (relocation, threshold-disabled, key-absent, AddDefaultValue, already-on-disk, engine-level integration test). No Critical findings; security scan attributed nothing to this diff (all SCA/secret findings are pre-existing go.mod/fixture baseline — go.mod is untouched here).

Should-Fix (see inline):

• New nil-config panic path: applyConfiguration(nil) via WithConfig(nil)/InvokeWithConfig(id, nil) — one-line guard if config == nil { return }.
• Engine silently overrides a workflow's explicit WithConfiguration on returned Data — decide/​document the "engine config wins last" contract; add a test for the override case.

Suggestions (see inline):

• Relocation loop runs even when the callback returned an error → guard with if err == nil.
• WithConfiguration exported-API semantic change is untested for direct callers.

Minor (no inline):

• setPayloadLocation recomputes a full sha256 and populates Location.Sha256 for in-memory []byte outputs that stay in memory (below threshold) — a new per-Invoke hash pass for large in-memory payloads; consider hashing lazily after the size check.
• A failed spill in applyConfiguration (unwritable TEMP_DIR_PATH) is swallowed — payload silently stays in memory (the OOM case this PR targets) with no signal beyond the existing error log. Acceptable fallback, but now on the engine hot path.
• DataImpl has no mutex; applyConfiguration mutates payload fields after Invoke returns — safe unless a workflow shares/retains returned Data across goroutines. Worth a doc note.

— AI review