internal/cmd/start_test.go (1)

24-27: Spawn the built CLI with a deadline.

A hung subprocess here blocks the whole suite until the global go test timeout because CombinedOutput() is running on a plain exec.Command. Use exec.CommandContext with a timeout or a deadline derived from t.Deadline().

Suggested hardening

-func runBuiltCLICommand(th test.Command, extraEnv []string, args ...string) ([]byte, error) {
-	cmd := osexec.Command(th.Config.Paths.Executable, test.WithConfigFlag(args, th.Config)...)
+func runBuiltCLICommand(t *testing.T, th test.Command, extraEnv []string, args ...string) ([]byte, error) {
+	t.Helper()
+
+	ctx := context.Background()
+	if deadline, ok := t.Deadline(); ok {
+		var cancel context.CancelFunc
+		ctx, cancel = context.WithDeadline(ctx, deadline)
+		defer cancel()
+	}
+
+	cmd := osexec.CommandContext(ctx, th.Config.Paths.Executable, test.WithConfigFlag(args, th.Config)...)
 	cmd.Env = append(append([]string{}, th.ChildEnv...), extraEnv...)
 	return cmd.CombinedOutput()
 }
@@
-	output, err := runBuiltCLICommand(th, extraEnv, args...)
+	output, err := runBuiltCLICommand(t, th, extraEnv, args...)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/cmd/start_test.go` around lines 24 - 27, runBuiltCLICommand
currently spawns the subprocess via osexec.Command and calls CombinedOutput(),
which can hang the test runner; change runBuiltCLICommand to accept a context
(or *testing.T) and use exec.CommandContext with a context created from
t.Deadline() (or a sensible timeout) so the subprocess is killed on timeout,
then call cmd.CombinedOutput() on that command; keep the same env handling
(th.ChildEnv and extraEnv) and preserve the call sites by passing a context or t
through to runBuiltCLICommand.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/core/spec/loader.go (1)

256-298: ⚠️ Potential issue | 🟠 Major

LoadYAMLWithOpts does not preserve LocalDAGs from multi-document YAML.

When loading from raw YAML data (not file), this function only processes the first document via def.build(buildCtx). If the input contains multiple YAML documents (separated by ---), secondary documents that define sub-DAGs are silently dropped. The file-based loadDAG path at lines 396-404 correctly populates mainDAG.LocalDAGs, but this memory-based path does not.

This could cause issues when rebuilding DAGs from YamlData during retry/restart if the original DAG contained inline sub-DAGs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/spec/loader.go` around lines 256 - 298, LoadYAMLWithOpts
currently only builds the first parsed document via def.build(buildCtx), which
drops additional documents and therefore loses LocalDAGs; update the memory-path
in LoadYAMLWithOpts to iterate over all parsed documents (similar to the
file-path behavior that sets mainDAG.LocalDAGs), building each document with
def.build(buildCtx) and either merging or appending the resulting DAGs into
dest.LocalDAGs (and merging into dest where appropriate), while preserving
existing logic around baseDef/build errors, dest.YamlData and
dest.BaseConfigData; use the existing symbols def, buildCtx, baseDef, dest, dag
and the merge function to locate where to add the loop/aggregation.

internal/cmn/buildenv/env.go (1)

30-34: ⚠️ Potential issue | 🟠 Major

Security: Presolved env values are exposed in process environment.

This serializes presolved values (which may include secrets defined in env: blocks) into child process environments via PresolvedEnvPrefix+key. These values become visible in /proc/<pid>/environ, crash dumps, and debug tooling, increasing secret exposure surface.

Consider an alternative transport mechanism (temp file with restricted permissions, pipe, or file descriptor) for sensitive payloads.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/cmn/buildenv/env.go` around lines 30 - 34, The code currently
serializes presolved values into child process env using PresolvedEnvPrefix+key
(the loop building extra), which exposes secrets; change this to avoid putting
secrets in the environment: instead write the presolved payload to a temp file
with strict permissions (0600) or a pipe/FD and return only a single reference
(e.g., PRESOLVED_ENV_FILE) or file descriptor identifier to the child process;
update the function that builds extras (the loop over keys producing
PresolvedEnvPrefix+key entries) to (1) write entries[key] to a securely created
temp file or a dedicated FD, (2) add only a single env var like
PRESOLVED_ENV_FILE=<path> or pass the FD, and (3) ensure the file/FD is properly
cleaned up and access-restricted—do not place individual PresolvedEnvPrefix+key
entries into the environment.

internal/core/spec/runtime_env.go (1)

65-78: ⚠️ Potential issue | 🟠 Major

Prefer the persisted YAML snapshot over Location.

Line 66 currently reloads from dag.Location before dag.YamlData. If the file changed after the run was queued or persisted, env reconstruction will silently pick up newer on-disk content instead of the stored snapshot, which breaks retry/restart parity.

Suggested fix

 switch {
- case dag.Location != "":
- 	fresh, err := Load(ctx, dag.Location, loadOpts...)
- 	if err != nil {
- 		return nil, err
- 	}
- 	return append([]string{}, fresh.Env...), nil
-
 case len(dag.YamlData) > 0:
 	fresh, err := LoadYAML(ctx, dag.YamlData, loadOpts...)
 	if err != nil {
 		return nil, err
 	}
 	return append([]string{}, fresh.Env...), nil
+
+ case dag.Location != "":
+ 	fresh, err := Load(ctx, dag.Location, loadOpts...)
+ 	if err != nil {
+ 		return nil, err
+ 	}
+ 	return append([]string{}, fresh.Env...), nil
 
 default:
 	return append([]string{}, dag.Env...), nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/spec/runtime_env.go` around lines 65 - 78, The current switch
prefers dag.Location over dag.YamlData, causing on-disk changes to override the
persisted YAML snapshot; update the logic in the function that reconstructs the
environment to check dag.YamlData first and call LoadYAML(ctx, dag.YamlData,
loadOpts...) when len(dag.YamlData) > 0 (returning append([]string{},
fresh.Env...)), and only fall back to Load(ctx, dag.Location, loadOpts...) when
YamlData is empty, preserving retry/restart parity.

internal/core/spec/variables.go (1)

101-106: Semantic mismatch: EnvSourceDAGEnv used for presolved host-env values.

The presolved values from ctx.opts.BuildEnv originated from the host environment (via buildenv.Load()), not from the DAG's env: field definition. Using EnvSourceDAGEnv for these values is semantically incorrect.

Per internal/cmn/eval/envscope.go, EnvSourceDAGEnv is defined as "From DAG env: field". While this doesn't currently affect resolution (scope lookup ignores source), it could:

1. Cause confusion during debugging when tracing variable origins
2. Break future logic that uses EnvSource for precedence decisions

Consider adding a dedicated EnvSourcePresolvedBuild constant or using EnvSourceOS to accurately represent the origin.

♻️ Suggested approach

In internal/cmn/eval/envscope.go, add:

EnvSourcePresolved EnvSource = "presolved" // From presolved build env transport

Then in this file:

 			if presolved, ok := ctx.opts.BuildEnv[p.key]; ok {
 				value = presolved
-				scope = scope.WithEntry(p.key, value, eval.EnvSourceDAGEnv)
+				scope = scope.WithEntry(p.key, value, eval.EnvSourcePresolved)
 				vars[p.key] = value
 				continue
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/spec/variables.go` around lines 101 - 106, The presolved values
taken from ctx.opts.BuildEnv are annotated with EnvSourceDAGEnv but should be
marked as a distinct source; add a new EnvSource constant (e.g.,
EnvSourcePresolved or EnvSourcePresolvedBuild) to internal/cmn/eval/envscope.go
and then change the scope.WithEntry call in variables.go (the block referencing
ctx.opts.BuildEnv and presolved) to use that new EnvSource constant instead of
EnvSourceDAGEnv so the entry correctly reflects "presolved build env" as the
origin.

internal/cmd/start_test.go (1)

24-57: Extract the built-CLI/output helpers into internal/test.

runBuiltCLI* and statusOutputValue now duplicate logic that also exists in internal/runtime/subcmd_test.go and internal/intg/queue/queue_test.go. A small shared helper would keep env injection and key= trimming consistent across all three suites.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/cmd/start_test.go` around lines 24 - 57, Create a shared test helper
package (internal/test) and move the duplicate helpers into it: implement
exported functions RunBuiltCLICommand(th test.Command, extraEnv []string, args
...string) ([]byte, error), RunBuiltCLI(t *testing.T, th test.Command, extraEnv
[]string, args ...string) string and StatusOutputValue(t *testing.T, status
*exec.DAGRunStatus, key string) string that preserve the exact behavior (env
injection via th.ChildEnv + extraEnv, using test.WithConfigFlag for args,
require.NoError in RunBuiltCLI, and trimming the "key=" prefix from output in
StatusOutputValue); then replace the local implementations in
internal/cmd/start_test.go, internal/runtime/subcmd_test.go and
internal/intg/queue/queue_test.go with imports from internal/test and call the
new exported functions, removing the duplicated code.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes