Skip to content

feat: support shellcheck runner - #12

Draft
JasonColapietro wants to merge 1 commit into
VectorInstitute:mainfrom
JasonColapietro:feat/support-shellcheck-runner
Draft

feat: support shellcheck runner#12
JasonColapietro wants to merge 1 commit into
VectorInstitute:mainfrom
JasonColapietro:feat/support-shellcheck-runner

Conversation

@JasonColapietro

Copy link
Copy Markdown

Summary

  • classify ShellCheck as a supported runner
  • parse ShellCheck success, SC#### diagnostics, and ambiguous output
  • add focused regression coverage for all three paths

Testing

  • cargo fmt --check
  • git diff --check

cargo test and Clippy could not compile locally because required locked crates were unavailable offline; this draft relies on hosted CI for those gates.

Closes #3.

@amrit110

amrit110 commented Aug 4, 2026

Copy link
Copy Markdown
Member

Thank you for this. It is the first outside contribution to the project, and it is a good one.

What I like: you covered the pass, fail, and ambiguous cases separately, and you return Outcome::Unknown when the output cannot be read instead of guessing. That is exactly the behaviour this project wants. All six CI jobs are green.

Four things to fix before this leaves draft.

1. The evidence line shows a URL instead of the finding

ShellCheck prints every finding twice. Once as the diagnostic, and again at the bottom as a wiki link. Both contain the SC#### code, and .rev() finds the link first:

EVIDENCE = Some("https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing and word splitting.")

So the user sees a URL where they should see the offending line.

The test does not catch this, because the URL also contains SC2086.

Scanning forward and skipping the link fixes it, and gives you a count too:

let evidence = output
    .lines()
    .find(|line| diagnostic.is_match(line) && !line.contains("shellcheck.net"))
    .map(|line| line.trim().to_string());
let count = output
    .lines()
    .filter(|line| diagnostic.is_match(line) && !line.contains("shellcheck.net"))
    .count() as u32;
return (Outcome::Failed, None, Some(count), evidence);

And a stronger assertion:

let evidence = bad.evidence_line.as_deref().unwrap_or_default();
assert!(
    evidence.contains("SC2086") && !evidence.contains("shellcheck.net"),
    "evidence should quote the finding, not the wiki URL: {evidence}"
);

2. Please rebase on main

main has moved since you branched. The conflict is not a real disagreement. The test next to yours, suppressed_failure_is_caveated, was renamed and rewritten, and your new test landed on top of it. Keeping both sides resolves it.

git fetch upstream && git rebase upstream/main

I tried this locally. With the fix above, all 93 tests pass.

3. Change Closes #3 to Part of #3

#3 is the running checklist for every missing runner. Merging this would close it while most of the list is still open. Ticking the ShellCheck box there would be great.

4. Add ShellCheck to the README table

People check the Runners it understands table to decide if the tool works for them. One entry in the Lint row.

Not a blocker

shellcheck --severity=error on a file with only info findings will still report Failed, because the parser looks for an SC#### code and ignores the severity. ShellCheck itself exits non-zero for any finding, so this is fine to merge as is. If you want it, the severity is in the diagnostic (SC2086 (info)), and honouring an explicit -S floor would make a nice follow-up.

Thanks again for picking this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for more test runners and linters

2 participants