Skip to content

ci: correct SC2016 comment in crush workflow - #299

Merged
taoeffect merged 1 commit into
masterfrom
clarify-spellcheck-comment
Aug 4, 2026
Merged

ci: correct SC2016 comment in crush workflow#299
taoeffect merged 1 commit into
masterfrom
clarify-spellcheck-comment

Conversation

@pedrogaudencio

Copy link
Copy Markdown
Collaborator

@taoeffect I investigated the spellcheck comment and concluded that make lint-actions runs actionlint, which invokes shellcheck only if the shellcheck binary is on PATH. Since it's not installed in the lint-actions job in CI this was never firing in CI. I added the comment previously in order to suppress (my) local runs that would otherwise flag it.

Closes #298

AI Disclosure

Co-authored with: Opus 5

@pedrogaudencio pedrogaudencio self-assigned this Aug 3, 2026
Copilot AI review requested due to automatic review settings August 3, 2026 12:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the inline ShellCheck (SC2016) suppression rationale in the “crush” pull-review workflow to reflect the correct threat model and functional behavior of the sandbox pre-flight check, aligning the workflow comment with Issue #298’s requirements.

Changes:

  • Reword the SC2016 suppression comment to explain that $GH_TOKEN must be expanded by the inner (sandboxed) shell.
  • Clarify the two real consequences of outer-shell expansion: a guaranteed false failure and exposure via the process command line.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pedrogaudencio

pedrogaudencio commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

/review


AI review started.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Advanced AI Review

  • Type: Agentic (crush)
  • Model: glm-5.2
Click to expand review

I'll review the changes. Let me first read the full context of the modified file.I've reviewed the diff and read the surrounding context. This is a pure comment update addressing issue #298, replacing an inaccurate threat-model explanation with a correct one. Let me verify the technical claims against the actual check code.The change is a comment-only update that correctly replaces a misleading threat model. I traced the logic: [ -z "$GH_TOKEN" ] at line 523 would indeed produce a false-positive failure if outer-expanded (the literal token is non-empty, so -z returns false, triggering fail=1), and the value would land in the sh process's argv (/proc/<pid>/cmdline). The new comment on lines 515-519 accurately describes both the functional and security risks, and correctly names $GH_TOKEN specifically rather than the vague "the variables" in the old text.

No 🔴, 🟡, or meaningful ⚪️ issues found. The comment is technically accurate, grammatically correct, and directly resolves issue #298. No changes to the code logic were made, so there is nothing to break or conflict.


Review generated using glm-5.2 via Z.AI. Comment /review to re-run.

@pedrogaudencio

Copy link
Copy Markdown
Collaborator Author

@taoeffect approved! ✅

@taoeffect
taoeffect merged commit f87d84d into master Aug 4, 2026
33 checks passed
@taoeffect
taoeffect deleted the clarify-spellcheck-comment branch August 4, 2026 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Correct misleading SC2016 comment in crush workflow

3 participants