Add RecordingRunner and Execute() test suite; fix cache/persist bugs

[kindermax]

Summary

• Implements #363: adds RecordingRunner test double and a comprehensive table-driven Execute() test suite in package executor
• Fixes three bugs surfaced during code review of the no-cache-for-mixins work:
  • Remote mixin download fallback was only available when --no-cache was set; it is now unconditional (consistent with ensureRemoteConfig), and both the download error and the cache-read error are surfaced when the fallback also fails
  • WithNoCache() alone only half-applied the flag — mixins were force-downloaded but the top-level remote config still read from cache; fixed by passing opts.noCache (not the raw positional bool) to ensureRemoteConfig
  • Mixin persist wrote directly to the cache file in-place, leaving it truncated on a mid-write crash; replaced with util.WriteFileAtomic (write-to-temp + rename), extracted from the duplicate writeCacheAtomic that already existed in load.go

Test plan

• go test ./internal/executor/... — all 11 new Execute() tests pass
• go test ./internal/config/... — existing load + config tests pass including no-cache integration tests
• go test ./internal/util/... — passes
• go build ./... — clean build

Summary by Sourcery

Add an atomic file write helper, use it for remote configs and mixin persistence, and introduce a RecordingRunner-based Execute() test suite for the executor.

Bug Fixes:

• Ensure remote configs respect the effective no-cache option instead of the raw parameter.
• Always fall back to cached remote mixins on download failure and surface both download and cache-read errors when fallback fails.
• Persist remote mixin configs and remote config cache files atomically to avoid partial writes on crashes.

Enhancements:

• Introduce a reusable WriteFileAtomic helper in the util package and replace duplicated atomic cache-writing logic.
• Add a RecordingRunner test double and comprehensive Execute() tests covering init, dependencies, after-scripts, parallel execution, and checksum behavior.

Tests:

• Add an executor Execute() table-style test suite using RecordingRunner to validate execution ordering, error propagation, parallel behavior, and checksum persistence.

[sourcery-ai]

Reviewer's Guide

Adds a RecordingRunner test double and a comprehensive Execute() test suite for the executor, while fixing caching and persistence behavior for remote configs and mixins and extracting a shared atomic file write helper.

Sequence diagram for updated remote mixin download and cache fallback behavior

sequenceDiagram
    participant Config
    participant RemoteMixin

    Config->>RemoteMixin: download(ctx, progress)
    alt downloadErr is nil
        RemoteMixin-->>Config: data
    else downloadErr is not nil
        RemoteMixin->>RemoteMixin: tryRead()
        alt readErr is not nil
            RemoteMixin-->>Config: error(downloadErr and readErr)
        else cachedData is not nil
            RemoteMixin-->>Config: cachedData
        else cachedData is nil
            RemoteMixin-->>Config: error(downloadErr)
        end
    end

Loading

Flow diagram for shared atomic file write helper usage

flowchart TD
    A[LoadRemote] --> B[ensureRemoteConfig]
    B --> C[util.WriteFileAtomic]

    D[RemoteMixin.persist] --> C

    C --> E[Cache file safely updated]
    C --> F[Remote mixin file safely persisted]

Loading

File-Level Changes

Change	Details	Files
Ensure remote config loading consistently honors no-cache and uses shared atomic file writes.	Pass opts.noCache instead of the raw noCache bool into ensureRemoteConfig so LoadRemote correctly applies the no-cache option across both mixins and top-level config Replace the local writeCacheAtomic helper with the shared util.WriteFileAtomic for writing the remote config cache file atomically Remove the now-duplicated writeCacheAtomic implementation from load.go	internal/config/load.go
Introduce a reusable atomic file write utility and use it for mixin persistence.	Add util.WriteFileAtomic which writes to a sibling temp file, sets 0o644 permissions, and renames atomically with appropriate error handling and cleanup Refactor RemoteMixin.persist to use util.WriteFileAtomic instead of truncating and writing the cache file in-place, and adjust errors to wrap the new helper	internal/util/file.go internal/config/config/mixin.go
Make remote mixin download fallback unconditional and surface both download and cache read failures.	Change readMixin to always attempt a cached fallback when remote mixin download fails, independent of the no-cache flag When both download and cache read fail, return an error that includes both underlying failures for better diagnostics Keep the download error as the returned error when no cached mixin data is available	internal/config/config/config.go
Add RecordingRunner test double and a table-driven Execute() test suite covering init, dependencies, parallelism, after-hooks, and checksum behavior.	Introduce a RecordingRunner ScriptRunner test double that records invocations and supports configurable per-call failures via indices Add helpers for building minimal config.Config, sequential and parallel commands, and executor Contexts for tests Add Execute() tests covering init script run-once semantics, dependency order and DependencyError chains, sequential vs parallel script dispatch, after-script behavior on success/failure, checksum env propagation into runner calls, and checksum persistence only on successful runs Ensure TestMain initializes env debug level to avoid panics in executor.runCmd during tests	internal/executor/execute_test.go

Possibly linked issues

• Add RecordingRunner and Execute() test suite #363: PR delivers RecordingRunner and the specified Execute() test suite in package executor, fully satisfying the issue.
• Add RecordingRunner and Execute() test suite #363: PR delivers the RecordingRunner and Execute() test suite required by the ScriptRunner-based executor testability issue.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've left some high level feedback:

• In readMixin, the combined error when both download and cache read fail only wraps the download error (%w) while printing the cache-read error with %v; consider wrapping the read error as well (e.g., via errors.Join or a second %w) so callers can programmatically inspect both failures.
• The new WriteFileAtomic helper is a good centralization of the atomic-write logic; you might want to audit and migrate any other direct os.WriteFile / OpenFile usages for config-like data to this helper to avoid partial writes elsewhere and keep behavior consistent.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `readMixin`, the combined error when both download and cache read fail only wraps the download error (`%w`) while printing the cache-read error with `%v`; consider wrapping the read error as well (e.g., via `errors.Join` or a second `%w`) so callers can programmatically inspect both failures.
- The new `WriteFileAtomic` helper is a good centralization of the atomic-write logic; you might want to audit and migrate any other direct `os.WriteFile` / `OpenFile` usages for config-like data to this helper to avoid partial writes elsewhere and keep behavior consistent.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}