* feat(executor): add easy GHA emulation fixes for better compatibility
- Expand github.* context with 13 missing env vars (CI, GITHUB_ACTIONS,
GITHUB_REF_NAME, GITHUB_REF_TYPE, GITHUB_REPOSITORY_OWNER, etc.) and
improve GITHUB_ACTOR to use git config / $USER instead of hardcoded value
- Enforce timeout-minutes at both job level (default 360m per GHA spec)
and step level via tokio::time::timeout
- Implement defaults.run.shell and defaults.run.working-directory with
proper fallback chain: step > job defaults > workflow defaults > bash
- Implement hashFiles() expression function with glob matching, sorted
file hashing (SHA-256), and integration into the substitution pipeline
* fix(executor): harden hashFiles, working-directory, and shell -e
Three issues from code review, all in the "we got the GHA emulation
*almost* right" category:
1. hashFiles() was returning an empty string when no files matched.
GHA returns the SHA-256 of empty input (e3b0c44...), not nothing.
An empty string as a cache key component is the kind of thing
that silently ruins your day. Also, unreadable files were being
skipped without a peep — now we at least warn about it.
2. The working-directory default resolution was doing a naive
Path::join with user-controlled input. If someone writes
`working-directory: ../../../etc` or an absolute path, join
happily replaces the base. Inside a container this is *somewhat*
contained, but in emulation mode it's a real path traversal.
Normalize the path and reject anything that escapes the
workspace.
3. The bash -e flag change (correct per GHA spec) was undocumented.
Scripts that relied on intermediate commands failing without
aborting the step will now break. Document it in
BREAKING_CHANGES.md so users aren't left guessing.
* fix(executor): complete the GHA shell invocation and harden hashFiles
The previous commit added `-e` to bash but stopped there, even
though the BREAKING_CHANGES.md *literally documented* the full GHA
invocation as `bash --noprofile --norc -e -o pipefail {0}`. So we
were advertising behavior we weren't actually implementing. This is
not great.
Without `-o pipefail`, piped commands like `false | echo ok` would
silently succeed, which is exactly the kind of divergence that makes
you distrust an emulator. And without `--noprofile --norc`, user
profile scripts can interfere with reproducibility.
While at it, fix hashFiles error handling — it was silently
swallowing read errors and producing a partial hash, which is worse
than failing because you get a *wrong* cache key with no indication
anything went sideways. preprocess_hash_files and
preprocess_expressions now return Result and the engine surfaces
failures as step errors.
Also add the tests that should have been there from the start:
shell invocation flags, working-directory path traversal rejection,
and defaults cascade (step > job > workflow).
* fix(executor): harden hashFiles, timeout, and shell edge cases
The previous round of GHA emulation fixes left a few holes that
would bite you in production:
hashFiles() would happily glob '../../etc/passwd' and hash whatever
it found outside the workspace. It also loaded entire file contents
into memory before hashing, which is *not great* when someone points
it at a large binary artifact. The glob patterns now reject '..'
traversal, and file contents are streamed into the SHA-256 hasher
via io::copy instead.
timeout-minutes accepted any f64 from YAML, including negative
values, NaN, and infinity — all of which make Duration::from_secs_f64
panic. Non-finite and non-positive values now fall back to the GHA
default of 360 minutes.
Unknown shell values were silently accepted with a '-c' fallback.
Now they emit a warning so you at least *know* something is off.
While at it, replaced the hash_files_read_error_returns_err test
that was testing two Ok paths (despite its name) with proper
path-traversal rejection tests.
* fix(executor): fix shadowed timeout_mins and extract sanitization helper
It turns out the job timeout error path was re-reading the *raw*
timeout_minutes value instead of using the already-sanitized one.
If someone set timeout-minutes to NaN or a negative number, the
sanitization would correctly fall back to 360, but the error
message would happily print "Job exceeded timeout of NaN minutes."
Not great.
Extract sanitize_timeout_minutes() so both the job and step
timeout paths use the same logic instead of duplicating the
is_finite/positive/clamp dance. While at it, add proper tests
for NaN, Infinity, negative, zero, and the max clamp — plus a
test that actually exercises the job-level timeout expiry branch,
which previously had zero coverage.
* fix: resolve 10 bugs found during full codebase review
- Fix memory leak from Box::leak() in status bar render loop
- Fix AES-GCM nonce reuse vulnerability in encrypted secret storage
- Fix Default impl for EncryptedSecretStore that discarded encryption key
- Fix early return in list command that prevented GitLab pipeline listing
- Fix double validation call in validate_github_workflow
- Wire --show-action-messages CLI flag through ExecutionConfig
- Add serde(rename = "if") to GitLab Rule if_ field for correct deserialization
- Fix potential panic on multibyte paths in workflow tab path shortening
- Include involved job names in circular dependency error messages
- Improve cron syntax validation to check value ranges, steps, and expressions
* fix: address PR review feedback
- Remove dead `_nonce` parameter from `EncryptedSecretStore::from_data`
- Add clarifying comment for inverted show/hide action messages mapping
- Add comprehensive cron validation tests (valid expressions, out-of-range
values, wrong part count, invalid steps, invalid ranges, edge cases)
* fix: resolve 27 bugs found during full codebase verification
A thorough manual verification of every feature uncovered a
*remarkable* collection of bugs hiding in plain sight. The
highlights:
The `strategy.matrix` YAML structure was never parsed. The Job
struct had `matrix` at the top level, but GitHub Actions nests it
under `strategy.matrix`. Serde silently ignored the `strategy`
key, so matrix expansion code existed but could never run. For
absolutely no reason. Introduce a proper `Strategy` struct and
wire it through the executor.
The Step struct was missing `if`, `id`, `working-directory`,
`shell`, and `timeout-minutes` fields. Step-level conditionals
were silently dropped — every step always ran regardless of its
`if` condition. While at it, `continue-on-error` was in the
struct but had no serde rename and was never checked during
execution. Fix all of that.
The validator cheerfully reported cyclic `needs` dependencies as
"Valid". Add DFS cycle detection so `A -> B -> C -> A` is caught
at validation time instead of blowing up at execution time.
Five of eight GitLab CI test fixtures failed to parse because the
model was too rigid: `extends` only accepted arrays (not strings),
`variables` rejected integers, `cache.key` rejected structured
formats, and `script` rejected single strings. Add custom
deserializers following the existing codebase pattern.
The GitHub trigger function leaked the auth token via curl process
arguments visible in `/proc/[pid]/cmdline`. Replace with reqwest,
matching the pattern already used elsewhere. Also add symlink and
path traversal protections in the executor.
Other fixes: hardcoded matrix variable stripping replaced with
proper substitution, `show_action_messages` wired through TUI,
dead `if true {}` removed, default branch detection uses remote
HEAD instead of current branch, cron validator accepts named
days/months, reusable workflow ref validation loosened from OR
to AND, matrix include entries merge into all matching combos.
* fix: harden step-level evaluation, volume checks, and add tests
The PR review turned up a few things that needed fixing before this
was actually ready.
The step-level `if` condition evaluator was silently reusing the
job-level `evaluate_job_condition` function, which knows nothing
about step-scoped expressions like `steps.<id>.outcome`, `success()`,
`failure()`, `always()`, or `cancelled()`. These would fall through
to the generic "unknown condition" path without so much as a warning.
Now they're detected early, a warning is logged, and they default to
true — which is at least *honest* about the limitation.
The volume path traversal check (`..`) was applied to the entire
volume spec string, meaning a perfectly legitimate container path
like `/safe/host:/container/..weird` would get rejected. The check
now only inspects the host path component after splitting on `:`,
which is the part that actually matters for traversal attacks.
While at it, renamed the awkwardly-named `step_name_for_skip` to
just `step_name` in `execute_matrix_job` for consistency with
`execute_job`, and added a BREAKING_CHANGES.md documenting the
EncryptedSecretStore serialization format change.
Added 19 new tests covering matrix include/exclude merge semantics,
step condition evaluation for unsupported expressions, volume path
traversal edge cases, and continue-on-error + step-level if parsing.
* fix: correct condition defaults, path traversal check, and null variable handling
The previous commit defaulted *all* unsupported step-level
condition functions (failure(), cancelled(), always(), success())
to true. It turns out that defaulting failure() and cancelled()
to true is semantically wrong — it means steps guarded by
`if: failure()` will *always* run, even when nothing failed.
That's not a feature, that's a bug.
Default each function to its most likely state: always() and
success() return true, failure() and cancelled() return false.
Not perfect (we still can't track actual step outcomes), but at
least we're not silently running cleanup steps on every build.
The path traversal check was using `contains("..")` which is a
substring match. A directory literally named `..hidden` would
get rejected. Use Path::components() to detect actual ParentDir
components instead of playing string matching games.
While at it, fix deserialize_variables in the GitLab models to
handle YAML null values as empty strings instead of producing
"~\n". Also trim the catch-all serialization output.
* fix: correct cycle detection, condition evaluation, and matrix continue-on-error
The DFS cycle detector in `dfs_detect_cycle` had a genuinely nasty bug:
when a cycle was found, it returned early *without popping itself from
rec_stack*. This left stale entries that corrupted the stack for
subsequent DFS traversals. Net result: cross-edges to already-visited
nodes would be falsely reported as cycles. A→B→A is a cycle, but
D→E→A is just a cross-edge. The old code couldn't tell the difference.
Fix this properly by introducing a separate `in_stack` HashSet for O(1)
membership checks, while keeping the Vec for path reconstruction. Both
are now correctly cleaned up — no early returns skip the cleanup.
While at it, `execute_matrix_job` was silently ignoring `continue-on-error`
on the Err branch. The non-matrix `execute_job` handled it correctly,
but the matrix path would just abort the entire job. Copy-paste bugs
are fun like that. Let's fix that.
The `evaluate_job_condition` status function handling was doing sequential
`contains()` checks with early returns, which meant compound expressions
like `failure() || success()` would match `failure()` first and return
false. Now we scan for all status functions in one pass and pick the
most permissive default when positive functions are present.
Also: `convert_yaml_to_step` was hardcoding `None` for `if_condition`,
`id`, `working_directory`, `shell`, and `timeout_minutes` despite the
YAML potentially having them. And `is_valid_cron_atom` was rejecting
valid POSIX cron syntax like `5/2`.
* refactor(executor): extract step guards into shared helper, fix steps.* default
The step-level if-condition check and continue-on-error handling was
copy-pasted between execute_job and execute_matrix_job with subtly
different control flow — one sets job_success=false and breaks, the
other returns Ok(JobResult{Failure}) immediately. Two copies of the
same logic that *already* disagree is not redundancy, it's a bug
waiting to happen. Let's fix that.
Extract run_step_with_guards() that encapsulates the if-condition
evaluation, execute_step call, and continue-on-error wrapping into
a single StepOutcome enum. Both job execution paths now call this
shared helper.
While at it, fix the condition evaluator defaulting bare steps.*
references to true — "steps.build.outcome == 'failure'" should
*not* optimistically run the step. Now only always() and success()
default to true; everything else (bare step refs, failure(),
cancelled()) conservatively defaults to false.
Also add serde alias "matrix" on Job.strategy so old workflows with
flat matrix: at job level still parse, and document the intentional
or_insert_with in matrix include merging per GitHub Actions spec.
* fix: clean up review findings in step guards, secret store, and test fixture
The PR review flagged three issues worth fixing before merge.
First, run_step_with_guards had a bogus StepStatus::Skipped check
in the abort_job logic. The condition tested for Failure *or*
Skipped, then only actually aborted on Failure — meaning the
Skipped branch did nothing except confuse anyone reading the code.
Simplify to just check Failure directly.
Second, EncryptedSecretStore::from_json would silently fail with a
generic serde error when fed the old serialization format (which
had a shared top-level nonce field). Now it detects the old format
by checking for the "nonce" key and returns a clear error pointing
at BREAKING_CHANGES.md. Added a test for this.
Third, tests/workflows/continue-on-error-test.yml was an orphan
fixture — nothing referenced it. The same content is already
tested inline by parse_continue_on_error_workflow in the parser.
Removed it.
* fix: correct cron day-of-week range, steps. false positive, and Step boilerplate
Three issues from PR review, all straightforward:
The cron validator was rejecting day-of-week value 7, which is a
perfectly valid Sunday alias in both POSIX cron and GitHub Actions.
The max was 6 when it should be 7. The named-value resolver guard
also needed updating from `max == 6` to `max >= 6` so named days
still resolve correctly with the wider range.
The `evaluate_job_condition` heuristic for detecting `steps.*`
references was using a bare `contains("steps.")`, which means an
env var like `env.MY_STEPS_COUNT` would falsely trigger it and
short-circuit to false. Now we check that the character before
"steps." is either start-of-string or non-alphanumeric. Not a
full expression parser, but it stops the obvious false positives.
While at it, add a `Step::with_run` constructor so the GitLab
converter doesn't need three identical 12-field struct literals
that silently break every time someone adds a field to Step.
* fix: harden steps. boundary check, document condition semantics, dedup cycles
The steps. word-boundary heuristic in evaluate_job_condition was
checking for alphanumeric characters before "steps." to avoid false
positives on env vars like "env.MY_STEPS_COUNT". It turns out that
underscore is *not* alphanumeric, so "env._STEPS_CHECK" would
incorrectly trigger the step-reference path and return false.
While at it, the always() && failure() compound expression returning
true got a proper comment explaining *why* that's intentional — we
lack step-status context locally, so we'd rather over-run than
silently skip steps. Not ideal, but honest.
The DFS cycle detector in detect_cyclic_needs could report the same
cycle multiple times depending on HashMap iteration order. Normalize
cycles by rotating the node list to start at the lexicographically
smallest node, then deduplicate via a HashSet. Same cycle from
different entry points now gets reported exactly once.
* fix: squash review nits — double parse, clippy warnings, lost flag
Three leftover issues from the codebase review PR:
The from_json() deserialization was parsing the JSON *twice* — once
into serde_json::Value to sniff for the old nonce field, then again
from the raw string into the actual struct. Parse once, use
from_value() on the already-parsed Value. Not rocket science.
The cycle detector had two clippy warnings: .iter().cloned().collect()
on a slice (just use .to_vec(), please) and .min_by_key() cloning a
double reference instead of comparing properly. Switch to .min_by()
with an explicit cmp.
The show_action_messages flag was being silently dropped in
execute_workflow_cli — hardcoded to false regardless of what the user
asked for. Propagate it through the function signature and the TUI
fallback path so it actually does something.
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 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.
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 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 custom deserializer for runs-on field to handle both string and array formats
- Update Job struct to use Vec<String> instead of String for runs-on field
- Modify executor to extract first element from runs-on array for runner selection
- Add test workflow to verify both string and array formats work correctly
- Maintain backwards compatibility with existing string-based workflows
Fixes issue where workflows with runs-on: [self-hosted, ubuntu, small] format
would fail with 'invalid type: sequence, expected a string' error.
This change aligns with GitHub Actions specification which supports:
- String format: runs-on: ubuntu-latest
- Array format: runs-on: [self-hosted, ubuntu, small]
- Copied schema files into parser crate src directory
- Updated include_str! paths to be relative to source files
- Ensures schemas are bundled with crate during publish
- Resolves packaging and verification issues during publication
Fixes the build error that was preventing crate publication.
- Updated include_str! paths from ../../../ to ../../../../
- This resolves packaging issues during cargo publish
- Fixes schema loading for parser crate publication
- Add support for job-level if conditions with basic expression evaluation
- Support both string and array formats for job needs field (needs: job vs needs: [job])
- Add missing job fields: if_condition, outputs, permissions to Job struct
- Implement job condition evaluation in executor with pattern matching for:
- Simple boolean conditions (true/false)
- GitHub event conditions (github.event.pull_request.draft == false)
- Job output conditions (needs.jobname.outputs.outputname == 'value')
- Jobs with false conditions are now properly skipped with appropriate logging
- Fixes parsing issues with workflows that use changes jobs and conditional execution
Resolves compatibility with workflows like iceoryx2 that use path filtering patterns.
This commit adds full support for GitLab CI/CD pipelines:
- Add GitLab CI pipeline models with complete spec support (jobs, stages, artifacts, cache, etc.)
- Implement GitLab CI/CD pipeline parsing and validation
- Add schema validation against GitLab CI JSON schema
- Support automatic pipeline type detection based on filename and content
- Add GitLab-specific CLI commands and flags
- Implement pipeline conversion for executor compatibility
- Add validation for common GitLab CI configuration issues
- Update CLI help text to reflect GitLab CI/CD support
- Support listing both GitHub and GitLab pipeline files
This expands wrkflw to be a multi-CI tool that can validate and execute both GitHub
Actions workflows and GitLab CI/CD pipelines locally.
- Extracted functionality from the `src/` directory into individual crates within the `crates/` directory. This improves modularity, organization, and separation of concerns.
- Migrated modules include: models, evaluator, ui, gitlab, utils, logging, github, matrix, executor, runtime, parser, and validators.
- Removed the original source files and directories from `src/` after successful migration.
- This change sets the stage for better code management and potentially independent development/versioning of workspace members.
