feat: [TKC-6403] exchange data between test workflows run as a suite - #8066
Merged
Conversation
Workflows composed into a suite through `execute.workflows` could not pass
anything back to the parent: the toolkit polled the child execution and threw
away everything but the status. Values and files had to travel through external
state.
A child now publishes with the same mechanism steps already use - writing to
/testkube/outputs - and the parent reads it back with the execution() expression:
execute:
workflows:
- name: producer
as: p
fetch:
- paths: ['results/**']
to: /data/from-producer
...
shell: echo '{{ execution("p").outputs.token }}'
The same function resolves "parent" from the execution ancestry, so a child can
read what scheduled it, and sibling exchange falls out of feeding one child's
output into the next child's config.
- step outputs are promoted to the execution record, so they cross the pod
- pkg/executiondata owns the registry, the expression functions and the
artifact transfer, modelled on the existing credential() machine
- `as` gives an entry a stable reference; two entries claiming the same one is
an error rather than an ambiguous winner
- execute specs are finalized when their operation starts, not up-front, so a
later entry can read an earlier one
- read_artifact() returns small files inline (1 MiB cap); `fetch` writes larger
payloads to disk
Files need read access to artifact storage, which the control plane did not
grant: ListExecutionArtifactsPresigned is new in proto/service.proto and
implemented for OSS. The Enterprise control plane needs the same RPC before
read_artifact() and fetch work there; values work everywhere today.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The suite fixture showed read_artifact() only from the parent reading its children. The child side is the direction that needs the "parent" reference and a control plane round-trip, so cover it too: the suite uploads a fixture file before scheduling anything, and the consumer reads it back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A workflow only registers the test workflows it ran itself, so a child of a suite
could not reach another child: execution("p") failed with an unknown reference,
and there was no way to name the sibling at all.
Both directions already fall through to the control plane when the reference is
an execution id, so the parent passing that id down is enough - what was missing
was permission to use it, which is lifted in
kubeshop/testkube-cloud-api@212a5f831's follow-up.
execute:
workflows:
- name: consumer
config:
producerId: '{{ execution("p").id }}'
# inside the consumer
{{ execution(config.producerId).outputs.token }}
{{ read_artifact(config.producerId, "results/summary.json") }}
The unknown-reference error now says that anything the workflow did not run must
be addressed by id, rather than only suggesting an earlier step - advice that was
misleading inside a child, which never runs anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Contributor
Greptile SummaryThe PR enables child TestWorkflow executions to expose outputs and artifacts to parent and sibling workflows, with stable execution aliases and deferred expression evaluation.
Confidence Score: 5/5The PR appears safe to merge based on the current follow-up scope. No blocking failure remains from the previously reported execution-reference, sensitive-output, short-secret, or withheld-environment issues. Important Files Changed
Sequence DiagramsequenceDiagram
participant Parent as Parent Workflow
participant Toolkit as Execute Toolkit
participant Child as Child Workflow
participant Record as Execution Record
participant Storage as Artifact Storage
Parent->>Toolkit: Schedule child with alias
Toolkit->>Child: Execute resolved workflow
Child->>Record: Publish non-sensitive outputs
Child->>Storage: Upload artifacts
Toolkit->>Record: Poll terminal result and outputs
Parent->>Record: execution(alias).outputs
Parent->>Storage: read_artifact() or fetch
Reviews (12): Last reviewed commit: "Potential fix for pull request finding" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Two defects in the suite data exchange, both found in review.
An entry combining `as` with a selector aborted the whole step. The reference was
claimed once per matched workflow, so the second match reported a duplicate and
failed the command before anything was scheduled. An aliased entry names one
group regardless of how many workflows it matched - the instances were already
told apart by execution("<alias>", index) at runtime, only the validation
disagreed. The claim now happens once per entry, and every matched name is still
claimed individually when there is no alias.
A step output holding a sensitive value reached the parent mangled. The
instruction that publishes outputs rides the log stream, which the runner
obfuscates with ShowLastCharacters, so a token arrived as ****ue - a value that
looks real, compares unequal, and gives no hint why. Such outputs are now
withheld from the execution record with a warning, and stay usable within their
own workflow through step.<id>.outputs.
Withholding rather than bypassing the obfuscator is deliberate: publishing raw
would put any credential a workflow writes to its outputs directory into the pod
log, and no output is worth trading that for. Credentials belong in credential().
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Signed-off-by: Vladislav Sukhin <vladislav@kubeshop.io>
vsukhin
changed the title
Signed-off-by: Vladislav Sukhin <vladislav@kubeshop.io>
Signed-off-by: Vladislav Sukhin <vladislav@kubeshop.io> # Conflicts: # pkg/cloud/service.pb.go
Collaborator
Author
|
@greptile check if service.pb.go is properly generated |
An output whose value holds a sensitive word cannot travel through the obfuscated log stream without being corrupted, so it is not published outside the workflow that produced it. Omitting it left the reader with nothing: a missing key resolves to an empty value, so a parent workflow silently configured a child with "" instead of the token it asked for. Publish a marker in its place. The real value still never leaves the workflow, but a reader now resolves something self-describing, and the two steps that would consume it - handing configuration to another workflow, and running a command - refuse to run with it instead of passing the marker on. Also hold sensitive words added at runtime to the same minimum length as the ones read from the environment. A one-character word matches nearly every line, which masked unrelated logs and withheld unrelated step outputs wholesale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Outputs cross the boundary between executions through the execution record, which they reach by being printed to the obfuscated log stream. An output holding a sensitive word therefore cannot be exchanged: it stays inside the workflow that produced it, and what leaves is a marker naming what was withheld. That was implemented but never written down, so it read as an unfinished feature rather than a decision. Say it where the feature is documented, with both reasons it is a decision - publishing the value would either corrupt it or leak it into a record anyone who can read the execution can read - and name the channels that can carry such a value instead. Pin it from the consumer side too: the test now asserts that a withheld output does not resolve to an empty value, and that the error names both the output to stop relying on and a channel that can carry it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@greptile updated contract |
The guard covered command arguments only, so a withheld output assigned to a computed environment variable slipped through: UseEnv resolved the marker like any other value and installed it, and the tool received it as the value of the variable with nothing looking at it again. Gate every install instead, plain values as well as computed ones. A step that spawns workers resolves their specification itself, so the marker reaches a worker baked in as a literal rather than as something to compute - guarding only the computed branch would have missed it. Guard the parallel step at its own resolution too. Its workers were receiving the marker inside their specification, and failing before anything is spawned names the missing output once instead of once per worker. Two sites that resolve the same expressions are deliberately left alone: tarball patterns, where a marker yields an empty glob rather than a wrong value, and the retry condition, which is consumed as a boolean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
An aliased entry stays addressable by the workflow it ran, which the
reference validation does not reserve - it claims the alias only. An
aliased selector covering workflow "a" and a separate unaliased entry
running "a" therefore both answered to execution("a", 0), and Lookup
returned whichever had been inserted first. Configuration built from it
could read the outputs of the wrong child, with nothing to indicate it.
Report it. Lookup now fails when a reference and position address more
than one execution, naming both so the author can see which runs collided
and say which was meant.
Validating it at claim time instead would have been smaller, but it would
also refuse to schedule two aliased entries running the same workflow -
told apart by their aliases, which is a topology worth keeping. The
workflow name is the only reference that becomes ambiguous there, and it
now fails when it is used rather than when it is created.
Addressing an aliased execution by its workflow name stays supported; it
is only a collision on that name that is refused.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Conflict: pkg/cloud/service.pb.go. Both sides had regenerated it - main added scheduler_policy to ExecutionTarget, this branch added the artifact-read RPCs - and the conflicts fell inside the serialized rawDesc byte array, where merging by hand would leave a corrupt descriptor. proto/service.proto merged cleanly with both sides' additions, so the generated file was regenerated from it with the pinned toolchain (buf v1.68.1, proto/buf.gen.old.yaml, protoc-gen-go v1.32.0, grpc-go v1.2.0). Regeneration also rewrote pkg/logs/pb/* and pkg/cloud/service_grpc.pb.go identically to what was already committed, which is the check that the toolchain matches; those were restored to avoid line-ending-only noise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nge' Another session merged main into this branch at the same time, from an older main - its merge brought edb32b4 only, without the scheduler policy commit that regenerates pkg/cloud/service.pb.go, so it had no conflict to resolve and no content this branch does not already carry. Merged rather than overwritten, so that its commit stays in the history. The tree is unchanged by it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Downloading an artifact talks to object storage directly, and it was the only storage path in the worker that verified the certificate. Storage in a self-hosted deployment commonly presents one signed by a private CA that the worker image has no reason to trust, so read_artifact() and fetch failed against the very storage the workflow had just uploaded to - uploading an artifact, and uploading logs, already skip verification. Take the client from the caller instead of hardcoding one, and build it from the same constant that configures the control plane client, so the two cannot disagree about a host they both talk to. A caller that passes nothing keeps verifying: a deployment that needs otherwise says so. The transport is a clone of the default rather than the default itself. Assigning to the shared one - as the log upload does - makes every other client in the process skip verification too. Tested against a TLS server with an untrusted certificate, which is the failure this fixes, in both directions: skipping verification reads it, verifying refuses it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
…family The dashboard groups output instructions by name, reading anything matching ^testworkflow(-.*)?$ as the status of a single child execution. The parent's registry instruction was named testworkflow-execution.<alias>, which matches - and it carries a list of executions rather than one, so every field the dashboard looked for came back undefined. It invented a nameless child stuck at Queued for each step that ran a workflow, next to the real ones. Name it executiondata.<alias> instead. Renaming here fixes every dashboard already deployed, where narrowing the pattern would only fix the next one, and the name is free to change while the feature is unreleased. The test now asserts the name stays out of both instruction families, since the reason for it lives in another repository. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
courageousillumination
approved these changes
Aug 24, 2026
courageousillumination
left a comment
courageousillumination
left a comment
Contributor
There was a problem hiding this comment.
Overall LGTM. The handling of sensitive data makes sense and the execution data + artifact split looks good.
Contributor
There was a problem hiding this comment.
Pull request overview
Enables data exchange between TestWorkflows composed as a suite by promoting step outputs to execution-level outputs (cross-pod/cross-execution) and adding mechanisms to read sibling/parent execution outputs and artifacts via the expression language and new control plane RPCs.
Changes:
- Introduces
pkg/executiondata(registry, expression functionsexecution()/read_artifact(), artifact download helpers, withheld-output markers) to read other executions’ outputs/artifacts safely. - Adds artifact listing via a new gRPC RPC
ListExecutionArtifactsPresignedplus storage support, and wires it through the control plane client/server. - Fixes step id propagation in stage flattening so outputs directories are prepared/scanned correctly; adds suite YAML + tests covering new behaviors.
Reviewed changes
Copilot reviewed 53 out of 57 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/suites/special-cases/data-exchange-suite.yaml | End-to-end suite fixture for exchange |
| proto/service.proto | Adds artifact listing RPC |
| pkg/testworkflows/testworkflowprocessor/stage/groupstage.go | Propagates step id to first stage |
| pkg/testworkflows/testworkflowprocessor/stage/groupstage_test.go | Tests id propagation in flatten |
| pkg/storage/storage.go | Extends storage interface for listing |
| pkg/storage/storage_mock.go | Updates mock for new method |
| pkg/storage/minio/minio.go | Implements ListFilesFromBucket |
| pkg/runner/executionsaver.go | Persist outputs during execution |
| pkg/mapper/testworkflows/openapi_kube.go | Maps as and fetch API→CRD |
| pkg/mapper/testworkflows/mappers_test.go | Updates mapping test expectations |
| pkg/mapper/testworkflows/kube_openapi.go | Maps as and fetch CRD→API |
| pkg/executiondata/withheld.go | Withheld-output marker + detection |
| pkg/executiondata/withheld_test.go | Tests withheld marker behavior |
| pkg/executiondata/types.go | Execution data model + outputs extraction |
| pkg/executiondata/repository.go | Control-plane-backed execution repository |
| pkg/executiondata/repository_test.go | Tests capability translation behavior |
| pkg/executiondata/registry.go | Registry for executed workflows |
| pkg/executiondata/registry_test.go | Tests lookup/ambiguity/grouping |
| pkg/executiondata/mock_repository.go | Mock for ExecutionRepository |
| pkg/executiondata/fetch_test.go | Tests artifact fetch-to-disk helpers |
| pkg/executiondata/expressions.go | Registers execution() / read_artifact() |
| pkg/executiondata/expressions_test.go | Tests expression resolution behaviors |
| pkg/executiondata/artifacts.go | Artifact download + safety checks |
| pkg/executiondata/artifacts_test.go | Tests read_artifact size/pattern cases |
| pkg/executiondata/artifact_client_test.go | Tests TLS skip-verify download |
| pkg/controlplaneclient/mock_client.go | Adds mock method for listing artifacts |
| pkg/controlplaneclient/execution_self.go | Implements client call for new RPC |
| pkg/controlplane/agent_grpc_general.go | Advertises artifact-read capability |
| pkg/controlplane/agent_grpc_execution.go | Implements ListExecutionArtifactsPresigned |
| pkg/controlplane/agent_grpc_execution_test.go | Tests new RPC behavior |
| pkg/cloud/service_grpc.pb.go | Generated gRPC stubs update |
| pkg/cloud/mock_cloud_api_client.go | Updates cloud API client mock |
| pkg/capabilities/capabilities.go | Adds tw-artifact-read capability |
| pkg/api/v1/testkube/model_test_workflow_step_execute_test_workflow_ref.go | OpenAPI model adds as + fetch |
| pkg/api/v1/testkube/model_test_workflow_step_execute_fetch.go | New OpenAPI model for fetch |
| k8s/helm/testkube-runner/charts/testkube-crds/templates/_generated_crds.tpl | CRD template includes as/fetch |
| k8s/helm/testkube-operator/charts/testkube-crds/templates/_generated_crds.tpl | CRD template includes as/fetch |
| k8s/helm/testkube-crds/templates/_generated_crds.tpl | CRD template includes as/fetch |
| k8s/crd/testworkflows.testkube.io_testworkflowtemplates.yaml | Generated CRD includes as/fetch |
| k8s/crd/testworkflows.testkube.io_testworkflows.yaml | Generated CRD includes as/fetch |
| cmd/testworkflow-init/runtime/machine.go | Adds executiondata machine wiring |
| cmd/testworkflow-init/runner/action_handlers.go | Publishes outputs to execution record |
| cmd/testworkflow-init/orchestration/setup.go | Separates mask vs classify; blocks withheld env |
| cmd/testworkflow-init/orchestration/setup_test.go | Tests sensitive sets + withheld env guards |
| cmd/testworkflow-init/data/stepmachine_test.go | Updates output scan tests; adds sensitivity partition tests |
| cmd/testworkflow-init/data/state.go | Adds output-prefix extraction helper |
| cmd/testworkflow-init/data/outputscan.go | Scan returns values; partitions sensitive outputs |
| cmd/testworkflow-init/data/executiondata.go | Builds registry + machine + repository helpers |
| cmd/testworkflow-init/data/client.go | Centralizes storage skip-verify constant |
| cmd/testworkflow-init/commands/run.go | Blocks running commands with withheld markers |
| cmd/tcl/testworkflow-toolkit/commands/parallel.go | Propagates executiondata machine + marker guard |
| cmd/tcl/testworkflow-toolkit/commands/execute.go | Defers exec spec finalize; records executions; fetch support |
| cmd/tcl/testworkflow-toolkit/commands/execute_test.go | Tests deferred finalization + alias claiming |
| api/v1/testkube.yaml | OpenAPI schema adds as/fetch |
| api/testworkflows/v1/zz_generated.deepcopy.go | Deepcopy for fetch structs |
| api/testworkflows/v1/step_types.go | Adds as and fetch to step types |
Files not reviewed (3)
- api/testworkflows/v1/zz_generated.deepcopy.go: Generated file
- pkg/cloud/mock_cloud_api_client.go: Generated file
- pkg/cloud/service_grpc.pb.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Lookup filtered by index before it looked at any reference, so an id was
found only when its position happened to match the index the caller
asked for. Since an unspecified index means 0, execution("<id>") reached
the first member of a fan-out group and nothing else.
An id names one execution outright, while an alias or workflow name
names a group whose members are told apart by index, so match ids first
and without consulting the index.
The two paths failed differently. execution("<id>") did not error - it
fell through to the control plane, which answers from the execution
record, and that record carries neither the alias nor the index. So a
shard read its own position as 0 and its alias as empty, at the cost of a
network call. 'fetch: from:' has no such fallback and failed outright.
The new expression test wires a repository that fails the test if it is
called at all, then asserts the index and alias come back - neither is
recoverable from the record, so they can only come from the registry.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vsukhin
requested review from
courageousillumination
and
a lite review from Copilot
August 25, 2026 11:29
'fetch: from:' looked only in the local registry, so it refused any
reference the workflow had not scheduled itself - including the sibling
execution id a suite hands down as configuration, which read_artifact()
accepted from the same workflow with the same id. The two ways of
reaching another execution's artifacts disagreed about what a reference
means.
Share the resolution instead of teaching fetch a second copy of it. The
registry-then-control-plane walk moves out of the expression machine into
a Resolver both use, so a reference resolves identically in either place
and 'from: parent' now works for the same reason execution("parent")
does.
Resolve takes a context, which the expression path cannot supply - an
expression function has none - but fetch can, so a cancelled fetch now
cancels the read behind it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 59 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- api/testworkflows/v1/zz_generated.deepcopy.go: Generated file
- pkg/cloud/mock_cloud_api_client.go: Generated file
- pkg/cloud/service_grpc.pb.go: Generated file
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
pkg/executiondata/registry.go:126
- The comment on Registry.Group says the result is "ordered by index", but the implementation just appends matches in insertion order without sorting. Either sort the slice before returning or adjust the comment so callers don't assume ordering.
cmd/tcl/testworkflow-toolkit/commands/execute.go:365 - When fetch.from cannot be resolved, the returned UnknownRefError isn't wrapped with the fetch index. In workflows with multiple fetch entries, this makes it harder to identify which fetch block failed.
if err != nil {
return errors.Wrapf(err, "fetch.%d", i)
}
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 59 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- api/testworkflows/v1/zz_generated.deepcopy.go: Generated file
- pkg/cloud/mock_cloud_api_client.go: Generated file
- pkg/cloud/service_grpc.pb.go: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/runner/executionsaver.go:118
- Race condition: watchResultUpdates() can incorrectly mark outputs as saved even if AppendOutput() appends new outputs while UpdateExecutionOutput is in flight. In that interleaving, AppendOutput() sets outputSaved=false, but the watcher later overwrites it with true, so newly appended outputs may not be persisted until End() (defeating the goal of making outputs readable while the workflow is still running).
Collaborator
Author
vsukhin
added a commit
that referenced
this pull request
Aug 26, 2026
The suite data-exchange work this branch was cut from has since landed on main as #8066, so the two histories describe the same feature and conflict where they disagree about it. One line does: exactArtifact's last return, reached when the control plane matched exactly one artifact whose path differs from the requested one. This branch answers with that artifact; main reports it as not found. Taking main's, since integrating a branch is no place to quietly reverse a decision made upstream - but see the note on the pull request, because the strict answer looks wrong for the wildcard case it exists to serve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Workflows composed into a suite through
execute.workflowscould not pass anything back to the parent: the toolkit polled the child execution and threw away everything but the status. Values and files had to travel through external state.A child now publishes with the same mechanism steps already use - writing to /testkube/outputs - and the parent reads it back with the execution() expression:
execute:
workflows:
- name: producer as: p fetch:
- paths: ['results/**'] to: /data/from-producer ... shell: echo '{{ execution("p").outputs.token }}'
The same function resolves "parent" from the execution ancestry, so a child can read what scheduled it, and sibling exchange falls out of feeding one child's output into the next child's config.
asgives an entry a stable reference; two entries claiming the same one is an error rather than an ambiguous winnerfetchwrites larger payloads to diskFiles need read access to artifact storage, which the control plane did not grant: ListExecutionArtifactsPresigned is new in proto/service.proto and implemented for OSS. The Enterprise control plane needs the same RPC before read_artifact() and fetch work there; values work everywhere today.
Pull request description
Checklist (choose whats happened)
Breaking changes
Changes
Fixes