mirror of
https://github.com/bahdotsh/wrkflw.git
synced 2026-09-02 04:02:34 +02:00
docs/cleanup-readmes
8 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Gokul
|
ebabe6414a |
feat(validate): cross-check local composite action required inputs (#78)
* feat(validate): cross-check local composite action required inputs The validate command was happily declaring workflows "valid" even when they used a local composite action without providing its required inputs. The workflow would then blow up at runtime, which is *exactly* the kind of thing a validator should catch. The problem was that validate_action_reference() only checked whether the local action path existed on disk. It never bothered to read the action.yml, look at the inputs section, or verify that required inputs were actually provided in the step's `with:` block. So it was doing about half its job. Thread the repo root path through the validator call chain (evaluate_workflow_file → validate_jobs → validate_steps → validate_action_reference) so we can resolve local action paths. Then read and parse the action.yml, extract inputs with `required: true` and no default, and flag any that are missing from the step's `with:` params. Case-insensitive matching because that's what GitHub Actions does. Graceful degradation: if we can't find the repo root or the action file is unreadable, we silently skip rather than blowing up. 10 new unit tests cover the various cases. Closes #67 * fix(validate): handle string `required` and drop unsafe cwd fallback Two bugs in the local action input validation that just landed: First, `find_repo_root` was falling back to `current_dir()` when no `.git` directory was found. This is *wrong* — if you're validating a workflow outside a git repo, the cwd could be literally anywhere, and you'd end up resolving local action paths against some random directory. Return `None` and skip the check instead. Second, `required: 'true'` (as a YAML string) was silently treated as not required, because we only checked `as_bool()`. GitHub Actions treats the string "true" as truthy, so we should too. Add case-insensitive string matching alongside the bool check. While at it, add a test for the string `required` case so we don't regress on this. |
||
|
Gokul
|
8a8d7e5eec |
fix: resolve correctness, security, and parsing bugs across codebase (#73)
* 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.
|
||
|
bahdotsh
|
422a035c40 |
test: add tests for review fixes and clean up dead code
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. |
||
|
bahdotsh
|
aa3366a797 |
fix: correct multiple bugs found during full codebase review
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. |
||
|
bahdotsh
|
f0b6633cb8 | renamed | ||
|
bahdotsh
|
48e944a4cc |
fix(validators): Add validation for duplicate step IDs within GitHub Actions jobs
GitHub Actions requires step IDs to be unique within each job scope, but wrkflw was not validating this constraint. This caused workflows with duplicate step IDs to pass validation with exit code 0, while GitHub would reject them with "The identifier 'X' may not be used more than once within the same scope". - Add HashSet tracking of step IDs in validate_steps() - Check for duplicate IDs and report validation errors - Use GitHub's exact error message format for consistency - Step IDs can still be duplicated across different jobs (which is valid) Fixes validation gap that allowed invalid workflows to pass undetected. |
||
|
bahdotsh
|
e73b0df520 |
feat(gitlab): add comprehensive GitLab CI/CD pipeline support
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. |
||
|
bahdotsh
|
470132c5bf |
Refactor: Migrate modules to workspace crates
- 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. |