Skip to content

fix: quote chmod paths and remove fragile sha256sum pipe in templates#43

Merged
jamesadevine merged 1 commit intomainfrom
devinejames/merlinbot-comments
Mar 17, 2026
Merged

fix: quote chmod paths and remove fragile sha256sum pipe in templates#43
jamesadevine merged 1 commit intomainfrom
devinejames/merlinbot-comments

Conversation

@jamesadevine
Copy link
Collaborator

Addresses merlinbot feedback on generated pipeline YAML:

Unquoted chmod pathchmod +x $(Pipeline.Workspace)/... could break if the workspace path contains spaces or glob characters. Now quoted consistently.

Fragile sha256sum pipesha256sum -c ... | grep -q ": OK" can mask a failing checksum without pipefail. Replaced with plain sha256sum -c ... which already exits non-zero on failure.

Changes

  • templates/base.yml: Fix 5 sha256sum pipes + 1 unquoted chmod
  • templates/1es-base.yml: Fix 3 sha256sum pipes + 1 unquoted chmod
  • tests/compiler_tests.rs: Update assertion to match new pattern

- Quote $(Pipeline.Workspace) in chmod +x to handle paths with spaces
- Replace sha256sum -c ... | grep -q with plain sha256sum -c ... which
  already exits non-zero on failure, avoiding masked errors without pipefail
- Update test assertion to match new checksum pattern

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jamesadevine jamesadevine merged commit b246fb1 into main Mar 17, 2026
3 checks passed
@github-actions
Copy link
Contributor

🔍 Rust PR Review

Summary: Changes are correct and well-motivated — one minor test quality note worth knowing.

Findings

✅ What Looks Good

  • sha256sum fix is correct: The old sha256sum -c ... | grep -q ": OK" pattern is genuinely fragile — without set -o pipefail, the pipeline exit code comes from grep, not sha256sum. The fix is right; sha256sum -c already exits non-zero on failure and prints a clear diagnostic.
  • chmod quoting is correct: Quoting $(Pipeline.Workspace)/... protects against paths with spaces or special characters. The surrounding echo "##vso[task.prependpath]$(Pipeline.Workspace)/..." is fine as-is since the outer double quotes already handle word splitting there.
  • Both templates updated consistently: All 8 occurrences (5 in base.yml, 3 in 1es-base.yml) were caught and fixed.

⚠️ Suggestions

  • tests/compiler_tests.rs:148 — The updated assertion template_content.contains("sha256sum -c checksums.txt --ignore-missing") would also have passed before this fix, because the old string "sha256sum -c checksums.txt --ignore-missing | grep -q \": OK\"" contains the new substring. The test change is semantically correct, but it means the test didn't actually guard against regression to the old pattern. A tighter assertion like !template_content.contains("grep -q") alongside the positive check would make the test a proper regression guard.

  • Pre-existing / not introduced here: sha256sum -c checksums.txt --ignore-missing exits 0 if all listed files are missing (the flag suppresses "no such file" errors entirely). If checksums.txt contains no entry matching the downloaded binary name, verification silently passes. This is worth a follow-up — a --strict flag or explicit post-check that the binary name appears in the file would close that gap. Not a blocker for this PR.

Generated by Rust PR Reviewer for issue #43 ·

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.

1 participant