The validate subcommand was calling std::process::exit(1) when a
directory couldn't be read, which is a rather aggressive response
to a permission error. Especially when the code four lines above
handles a *missing* path by setting validation_failed and moving
on to the next one. Consistency is nice. Let's have some.
Split the match from the method chain (because continue is a
statement, not an expression, and Rust has opinions about that)
and replaced the exit(1) with the same continue pattern.
While at it, slap #[must_use] on ContainerOutput so the compiler
will yell at anyone who discards a run_container result without
checking exit_code. All current callers already bind it, so this
is purely forward-looking — but the kind of bug it prevents is
the silent-misexecution kind, and those are nobody's favorite.
The previous commit fixed a bunch of bugs but left a few loose
ends. The next_job() function still had a redundant bounds check
that previous_job() already had cleaned up — the .filter() call
makes the inner `if workflow_idx >= self.workflows.len()` dead
code. Let's not leave half-finished refactors lying around.
While at it, add tests for the three behavioral changes that
*really* should have had tests from the start: emulation runtime
returning Ok on non-zero exit codes, log processor not panicking
on multi-byte UTF-8 near bracket boundaries, and step validator
correctly rejecting steps with only a name field.
Also fix formatting (cargo fmt) and a clippy warning about items
defined after the test module.
It turns out that build_image_inner() in docker.rs was calling
.elapsed() on a SystemTime to compute the tar mtime. That gives
you "seconds since modification" — which is *not* what mtime
means. Mtime is seconds since the Unix epoch. The fix is
.duration_since(UNIX_EPOCH) like a normal person would use.
While at it, the docker logs() call was passing None for options,
which means it wasn't actually requesting stdout or stderr. So
we were collecting logs from a stream that might not have any.
Explicitly set stdout: true and stderr: true.
The emulation runtime had a fun behavioral mismatch with Docker
and Podman: it returned Err on non-zero exit codes, swallowing
all stdout/stderr output. Docker and Podman return Ok with the
exit code and let the caller decide what to do. The engine
already handles non-zero exit codes in the Ok path, so the
emulation was just silently eating useful output for no reason.
The UI had a bounds check in next_job() that was mysteriously
absent from previous_job() — the kind of inconsistency that
waits patiently for someone to hit a stale workflow index and
get a panic. Added the same .filter() guard.
String slicing in the log processor wasn't checking char
boundaries, which is fine until someone's log contains a
multi-byte UTF-8 character before a bracket. Added
is_char_boundary() checks.
Step validation was accepting steps with only a 'name' field
and no 'uses' or 'run', which is not a valid step in GitHub
Actions. Fixed the validation to require at least one of the
two fields that actually *do* something.
Replaced .expect() calls on directory reads in main.rs with
proper error handling. Panicking because a directory isn't
readable is not great user experience.
It turns out that if someone writes `container:` with an empty image
string, we'd happily pass "" to Docker and let it figure out what
that means. Spoiler: it doesn't.
Similarly, volume specs like "/host:" or ":/container" would produce
a PathBuf::from("") mount, which is the kind of thing that makes
container runtimes *very* unhappy. Let's just skip those with a
warning instead of pretending they're valid.
While at it, replace the derived Serialize on ContainerCredentials
with a custom impl that redacts the password field. The Debug impl
was already doing this, but serde_json::to_string was still happily
dumping passwords in plaintext. Please don't do that.
ContainerCredentials had a derived Debug impl that would happily
dump passwords into logs, panic output, and anywhere else Debug
gets called. That's *exactly* the kind of thing that bites you at
3am when someone adds a debug trace and suddenly credentials show
up in plaintext in your log aggregator.
Replace the derived Debug with a manual impl that redacts the
password field. While at it, add a guard for empty volume specs
that would otherwise produce undefined Docker behavior, a note
about the splitn limitation with Windows paths, and fix clippy
warnings on the test assertions.
The remap_env_file closure had a fallback that would *invent* paths
like /github/workflow/github_output when the corresponding env key
didn't actually exist in job_env. Those paths point to nothing on
the mounted volume, so any step that tries to write to them gets a
lovely surprise.
Only remap keys that actually exist in job_env now. If GITHUB_OUTPUT
isn't set, we don't pretend it is.
While at it, volume mount options like :ro and :rw were being
silently stripped with no warning. A user specifying :ro expects a
read-only mount — silently giving them read-write is not great. Emit
a warning when we drop mount options, matching the existing pattern
in warn_unsupported_container_fields.
Add tests for both fixes plus container env precedence coverage.
The volume spec parser was using splitn(2, ':'), which means a
Docker volume like "/host:/container:ro" would produce a container
path of "/container:ro". That's not a path, that's a path with
garbage appended. splitn(3, ':') strips the options correctly.
The env path remapping was hardcoding filenames like
"/github/workflow/env" instead of deriving them from the actual
host paths. If environment.rs ever renames those files, the
remapping silently breaks and you get to debug phantom container
failures. Derive the filename from the real path instead.
While at it, add unit tests for prepare_container_mounts and
get_effective_runner_image — the two core functions from the
container directive work that had zero test coverage. Nine tests
covering Docker/Podman remapping, volume parsing (host:container,
single-path, :ro/:rw options), and the image selection fallback.
The container directive support in 2eae320 had ~45 lines of identical
volume-mounting and env-path-remapping code copy-pasted between the
Docker-action execution branch and the run-step branch. That's not
redundancy, that's a future bug waiting to happen in two places
instead of one.
Extract `prepare_container_mounts()` to handle the shared logic:
GitHub env file remapping, container-defined volume parsing, and the
runtime mode detection. Both branches now call into the same function.
While at it, fix single-path volume specs (e.g. "/data" without a
colon) which were being silently dropped because the code only
handled the `host:container` format. Now they mount at the same path
inside the container, which matches GitHub Actions behavior.
Also add `warn_unsupported_container_fields()` so users actually
*know* when their `options`, `credentials`, or `ports` fields are
being ignored rather than discovering it the hard way in production.
Add parser tests for `deserialize_container` covering string format,
full object format, absent container, and registry image with colon
in the tag.
It turns out that the Job struct in the parser had *no* container
field at all. When a workflow specified `container: alpine:3.22.1`,
serde silently dropped it, and the engine happily derived the runner
image from `runs-on` instead. So `apk add` runs inside Ubuntu.
Confusion ensues.
Add a JobContainer type with a custom deserializer that handles both
the string form (`container: alpine:3.22.1`) and the object form
(`container: { image: ..., env: ..., volumes: ... }`). A new
get_effective_runner_image() prefers the container image over the
runs-on mapping.
While at it, fix the GITHUB_ENV volume mounting for real container
runtimes. The old code identity-mounted the host temp path into the
container, which breaks on macOS with Podman because /var/folders
doesn't exist in the VM. Now we mount the github env directory at
/github/workflow/ and remap the env vars to match.
Container-level env vars and volumes are also wired through with
correct precedence (step > job > container).
Closes#58
It turns out that action references like `github/codeql-action/init@v3`
were being treated as if `github/codeql-action/init` was the repo name.
The resolver would then try to fetch action.yml from
`github/codeql-action/init/v3/action.yml` instead of the correct
`github/codeql-action/v3/init/action.yml`. Same bug hit shallow_clone
— it would try to clone a repo URL with the sub-path baked in, which
obviously doesn't exist.
Add a `sub_path` field to `ActionInfo` so `resolve_action` splits
`owner/repo/path@ref` into its actual components. The resolver,
cache key, and composite action clone all use the sub-path correctly
now.
While at it, stop using `std::env::set_var`/`remove_var` in the
wiremock tests. Those are unsound in multi-threaded test binaries
(Rust 1.83+ rightly marks them unsafe). Refactored `fetch_and_parse`
to accept the token as a parameter — the tests just pass it directly,
no env mutation needed.
Three issues from code review, all small but all real:
The echo fallback in execute_step was interpolating the `uses` string
directly into a single-quoted sh -c argument. A workflow with a
single quote in the action ref would break out of the shell string.
Escape single quotes with the standard '\'' pattern.
The fetch_and_parse tests were calling env::remove_var("GITHUB_TOKEN")
and env::set_var() without saving and restoring the original value.
If GITHUB_TOKEN was set before the test suite ran, it would be
permanently wiped for subsequent tests. All three tests now
save/restore properly.
While at it, document the ActionInfo::version field semantics —
it's empty for docker/local refs, holds the git ref for GitHub
action refs, and defaults to "main" when omitted. Future readers
shouldn't have to guess.
It turns out that resolve_action was blindly splitting on '@' for
*all* action references, including Docker image refs like
docker://alpine@sha256:abc123. The '@' in a Docker digest is not a
version separator — it's part of the image reference. Splitting it
produces a nonsensical repository and a fake "version" that happens
to be a SHA256 digest. Nobody noticed because the Docker path
doesn't use the version field, but the parsed data was still wrong.
While at it, the auth retry path in fetch_and_parse was constructing
a brand new reqwest::Client on every single 404-then-retry cycle.
That means a fresh TLS handshake each time, which is wasteful when
we already have a perfectly good static client pattern. Promote the
no-redirect client to a static Lazy, same as HTTP_CLIENT.
The auth redirect flow — where we send GITHUB_TOKEN to the origin
but strip it before following a redirect to a CDN — had zero test
coverage. This is the kind of security invariant that *really*
should not depend on code review alone. Add wiremock-based tests
that verify the token does not leak to redirect targets, plus tests
for the basic auth retry and 404 paths. Parameterize fetch_and_parse
with a base_url so wiremock can intercept the requests.
The PR review flagged three things that deserved fixing:
The action resolver was silently swallowing the *first* error when
action.yml failed and then retrying action.yaml. If action.yml
existed but had a parse error, you'd never know — it just quietly
tried the other filename. Now both error messages are combined so
you actually get useful diagnostics.
There was a stale comment in engine.rs that read "rest of the
existing code for handling regular actions" — which was left over
from the refactor and described absolutely nothing. Gone.
The SHA detection logic in shallow_clone was inline and untested.
Extract it into is_git_sha() and add proper tests covering valid
SHA-1, short hashes, branch names, tags, non-hex input, and
off-by-one lengths.
The action resolver was making HTTP requests to raw.githubusercontent.com
with no User-Agent header, which is the kind of thing that gets you
silently rate-limited by GitHub's CDN. Not great when your whole
resolution strategy depends on those requests actually succeeding.
While at it, the no-redirect policy on the authenticated retry path was
*correct* for preventing token leakage to non-GitHub hosts, but it also
meant that legitimate CDN redirects (3xx) would fall through to the
success check and produce a misleading "HTTP 301 fetching..." error.
Fix this by following the redirect with HTTP_CLIENT (no auth header)
when we get a 3xx, so we get the content without leaking the token.
Also add a note on the SHA-1 detection in shallow_clone — it only
matches 40-char hex strings, which will need updating if GitHub ever
adopts SHA-256 refs.
The exit_code branching in execute_step had a classic nested-condition
bug: the cargo-error detail block checked `exit_code != 0` *inside*
an `if exit_code == 0` block. That entire error path was unreachable
dead code. Confusion ensues.
Flatten the branching so the cargo-error path is actually reachable
on failure, and the verbose-output construction doesn't gate the
entire result.
While at it, fix two things in action_resolver: the BoundedCache
insert comment said "LRU order" when the eviction strategy is FIFO,
and the authenticated retry for private repos was reusing the shared
HTTP_CLIENT which follows redirects by default — meaning a
hypothetical redirect away from raw.githubusercontent.com would
happily forward the GITHUB_TOKEN to wherever it landed. Use a
no-redirect client for the authenticated request instead.
Three issues from code review, all minor but all worth fixing before
they confuse someone later.
The BoundedCache was documented as "LRU-style" when it's actually
plain FIFO — get() doesn't promote keys. Nobody cares for this use
case since actions resolve once per run, but calling FIFO "LRU" is
the kind of lie that breeds real bugs when someone trusts the docs
and adds access-pattern-dependent logic later. Fixed the comments.
The 404-retry-with-GITHUB_TOKEN pattern in fetch_and_parse was
correct but undocumented — it *only* targets raw.githubusercontent.com
so there's no token-to-attacker-host risk, but that's the kind of
thing you want a future reader to see immediately without having to
trace the URL construction. Added a comment.
resolve_action was setting version to "main" for docker:// refs and
local paths (./), which is semantically wrong. Docker refs embed
their tag in the repository string, and local paths have no version
at all. Neither value was ever *used* in those code paths, but a
wrong value sitting in a struct field is a bug waiting to happen.
Set version to "" for both cases instead.
The action resolver had a few problems that would bite in production.
The ACTION_CACHE was an unbounded HashMap behind a Mutex — so it
leaked memory indefinitely in long-running processes, and readers
blocked each other for no good reason. Replace it with a bounded
LRU-style cache (256 entries, oldest evicted first) behind an RwLock
so concurrent reads don't serialize.
shallow_clone() was using std::process::Command in async context,
which blocks the tokio runtime thread. For SHA refs that's *three*
sequential blocking operations. Convert the whole thing to
tokio::process::Command. While at it, add `--` before positional
args to prevent flag injection from crafted version strings, and
`--single-branch` to avoid fetching unnecessary refs.
The node version parser silently defaulted to 20 on malformed input
("nodefoo" -> node:20-slim). That's the kind of silent data
corruption that makes debugging a nightmare. Return an error instead.
HTTP timeout reduced from 15s to 5s — this is best-effort with a
fallback, so waiting 30s (two filenames × 15s) on a flaky network
is not helpful. GITHUB_TOKEN is now only sent on 404 retry instead
of unconditionally, because leaking tokens to public repos you don't
own is not great practice.
Also killed a dead conditional where both branches of an
if/else produced identical output.
It turns out that prepare_action() was returning the string "composite"
as if it were a Docker image name, and then execute_step() was checking
`if image == "composite"` to decide the control flow. This is not great.
Stringly-typed dispatch hiding inside what *looks* like an image name is
the kind of thing that confuses every future contributor.
Replace the String return with a proper PreparedAction enum that makes
the Composite vs Image distinction explicit at the type level. While at
it, fix several other bugs in the action resolver:
- git clone --branch doesn't work with SHA refs, and actions pinned to
full commit SHAs (a perfectly normal thing to do) would just fail with
a confusing git error. Extract a shared shallow_clone() helper that
detects SHA refs and uses init+fetch+checkout instead.
- DockerBuild actions (ones that bundle their own Dockerfile) were
silently falling through to determine_action_image(), which would
cheerfully return node:20-slim. Return an explicit error instead of
pretending everything is fine.
- Failed action.yml fetches were permanently cached as None, so a
transient network hiccup would poison the cache for the entire
process lifetime. Only cache successes now.
- The reusable workflow clone had the same --branch SHA bug; it now
uses the shared shallow_clone() helper too.
The previous commit added remote action.yml resolution, which was a
good idea in principle. But it had a *rather significant* problem:
when an action declares `runs.image: Dockerfile` (meaning "build my
bundled Dockerfile"), the resolver happily returned the literal
string "Dockerfile" as the Docker image name.
Confusion ensues. Downstream code tries to pull an image called
"Dockerfile" from a registry. That doesn't work.
Add a DockerBuild variant to ActionType for actions that bundle
their own Dockerfile. image_for_action() now returns Option<String>
— None for DockerBuild — so the caller falls back to the hardcoded
mapping instead of trying to pull nonsense from a registry.
While at it, fix several other problems from the initial PR:
- Reuse a static reqwest::Client instead of creating one per HTTP
request, because TLS initialization on every fetch is wasteful
- Capture git clone stderr instead of sending it to /dev/null, so
when cloning a remote composite action fails you actually get to
know *why*
- Add tests for ActionInfo.version parsing in the parser (the field
was added but never tested — please don't do that)
- Add edge-case tests for DockerBuild, unknown using values, missing
fields, and the docker://Dockerfile prefix variant
Fixes#48. When encountering third-party GitHub Actions, wrkflw previously
defaulted to node:20-slim for all unknown actions. Now it fetches the
action's action.yml from raw.githubusercontent.com, parses runs.using to
determine the action type (Node/Docker/Composite), and selects the
appropriate Docker image. Falls back to the existing hardcoded mapping on
any failure.
Add CLAUDE.md, AGENTS.md, and INDEX.md — all generated by the indxr
MCP tooling to give AI coding assistants a structured way to explore
the codebase without dumping entire files into context.
CLAUDE.md is the detailed version with token cost estimates and a
full tool reference. AGENTS.md is the condensed version. INDEX.md
is an auto-generated codebase index with file summaries and symbol
maps.
While at it, add .indxr-cache/ to .gitignore because nobody needs
that in the repo.
- Mount GitHub environment files directory containing GITHUB_ENV, GITHUB_OUTPUT, GITHUB_PATH, and GITHUB_STEP_SUMMARY
- Resolves Docker container exit code -1 when writing to $GITHUB_ENV
- Update volume mapping in both step execution contexts in engine.rs
- Tested on macOS with Docker Desktop
Closes: Issue where echo "VAR=value" >> "$GITHUB_ENV" fails in Docker runtime
- Fix clippy needless_borrows_for_generic_args warning
- Change &pid.to_string() to pid.to_string() for taskkill /PID argument
- Ensure clippy passes with -D warnings on Windows builds
- Remove unused io::self import from common scope
- Remove unused std::fs::OpenOptions and std::io::Write from windows_impl
- Add std::io import to unix_impl to fix io::Error references
- Ensure clippy passes with -D warnings on all platforms
- Add windows-latest to OS matrix with x86_64-pc-windows-msvc target
- Add dedicated Windows integration test job
- Verify Windows executable functionality
- Ensure cross-platform compatibility testing
This ensures Windows build issues are caught early in CI/CD pipeline.
- Document Unix vs Windows fd redirection limitations
- Update example to reflect platform-specific behavior
- Clarify that stderr suppression is Unix-only
- Add conditional compilation for Unix/Windows platforms
- Move nix dependency to Unix-only target dependency
- Implement Windows-compatible fd redirection API
- Preserve full functionality on Unix systems
- Add comprehensive documentation for platform differences
Resolves Windows build errors:
- E0433: could not find 'sys' in 'nix'
- E0432: unresolved import 'nix::fcntl'
- E0433: could not find 'unix' in 'os'
- E0432: unresolved import 'nix::unistd'
Closes#43
The 'name' field is optional per GitHub Actions specification. When omitted,
GitHub displays the workflow file path relative to the repository root.
This change removes the validation logic that incorrectly enforced the name
field as required, aligning the validator with the official JSON schema
which only requires 'on' and 'jobs' fields at the root level.
Fixes#50
- Add emulation support for dtolnay/rust-toolchain@ actions
- Include Rust and Cargo availability checks for dtolnay toolchain action
- Improve action detection logging for dtolnay Rust toolchain
Related to #49
- Fix get_runner_image() to map ubuntu-latest to ubuntu:latest instead of node:16-buster-slim
- Update ubuntu-22.04, ubuntu-20.04, ubuntu-18.04 to use proper Ubuntu base images
- Fix step execution to use action-specific images instead of always using runner image
- Update Node.js fallback images from node:16-buster-slim to node:20-slim
Fixes#49
