Repository navigation
fix(homebrew-alias): fail when a system_command cannot be converted - #51
Merged
Merged
Conversation
render rewrites only lines that start with system_command, so a call in any other position (an assignment such as result = system_command ...) was left in the install-step block, and Homebrew rejected the published cask as invalid. Fail the render instead, naming the lines, so the release stops before the tap is updated.
monit-reviewer
approved these changes
Oct 20, 2026
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: f695d9483d6d
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:repo-health | 0 |
| policies:conventions | 0 |
| automation:ci-deploy | 1 |
automation:ci-deploy (1 finding)
Minor - actions/homebrew-alias/modernize.sh:15
render() now captures the awk output through command substitution (
out=$(awk ... "$file")) and re-emits it withprintf '%s\n' "$out". Command substitution unconditionally strips all trailing newlines, so any cask that ends with a blank line, multiple trailing newlines, or no trailing newline will come out of render() with exactly one trailing\nregardless of the original. This contradicts the function's own contract ("Already-modern casks pass through unchanged") and matters downstream: actions/homebrew-alias/action.yml writes render's stdout straight to the tap file and gates the commit ongit diff --cached --quiet, so a cask whose only difference from the tap copy is trailing-newline shape would now produce a spurious diff and an unnecessary tap commit on every release, instead of the intended no-op. Preserve the original trailing-newline behavior, e.g. run the system_command grep check against the awk output directly (awk ... "$file" | tee >(grep ...)or write awk's output to a temp file and grep/cat that) instead of funneling it through a variable before re-printing it.
Reviewer Coverage
harness-engineering:repo-health— complete (constrained); skipped: none; constraints: nonepolicies:conventions— complete (constrained); skipped: none; constraints: noneautomation:ci-deploy— complete (broad); skipped: none; constraints: none
Inspected files (2)
actions/homebrew-alias/modernize.shactions/homebrew-alias/test_modernize.sh
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 54s | ~$1.19 (est.) | claude-sonnet-5 | cr 0.10.319
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | harness-engineering:repo-health, policies:conventions, automation:ci-deploy |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 2m 54s wall · 3m 41s compute |
| Cost | ~$1.19 (est.) |
| Pricing basis | anthropic-public-2026-09-23 |
| Tokens | 54 in / 14.6k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 8
- Out: 2.4k
- Cache read: 146.0k
- Cache create: 39.0k
- Cost: ~$0.21 (est.)
- Duration: 28s
harness-engineering:repo-health— claude-sonnet-5- In: 12
- Out: 2.2k
- Cache read: 277.3k
- Cache create: 39.7k
- Cost: ~$0.24 (est.)
- Duration: 33s
policies:conventions— claude-sonnet-5- In: 10
- Out: 1.4k
- Cache read: 216.9k
- Cache create: 38.4k
- Cost: ~$0.21 (est.)
- Duration: 20s
automation:ci-deploy— claude-sonnet-5- In: 18
- Out: 8.2k
- Cache read: 488.1k
- Cache create: 50.1k
- Cost: ~$0.38 (est.)
- Duration: 2m 05s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 474
- Cache read: 142.8k
- Cache create: 30.4k
- Cost: ~$0.15 (est.)
- Duration: 13s
…tical Command substitution strips trailing newlines, so a cask ending in blank lines came out changed. Write the rewrite to a temp file, check it, then print it.
piekstra
marked this pull request as ready for review
October 20, 2026 20:31
piekstra
added a commit
to open-cli-collective/codereview-cli
that referenced
this pull request
Oct 21, 2026
… install steps (#676) ## Summary The published `codereview-cli` cask has been invalid under Homebrew's install-step DSL since the catalog refresh was added to the postflight hook. Homebrew rejects it with `undefined method 'system_command' for an instance of Homebrew::InstallSteps::DSL`, so `brew upgrade` fails and `brew outdated` silently leaves cr out. The shared release workflow's modernizer turns each line that starts with `system_command` into a `run` step. The refresh line assigned the result (`result = system_command ...`) and the next line called `opoo`, so both stayed in the `postflight_steps` block. - Drop the install-time `cr catalog update`. The binary ships an offline catalog baseline, and install steps run sandboxed without network access by default, so the refresh has no working equivalent there. - Caveats point to `cr catalog update`. - `scripts/verify-package-render.sh` now requires every postflight line to be a bare `system_command` call, the form the modernizer converts. The companion change makes the modernizer fail on a call it cannot convert: open-cli-collective/.github#51 ## Validation - `goreleaser release --snapshot --clean --skip=publish,before` then `scripts/verify-package-render.sh`: OK. - The rendered cask, passed through `actions/homebrew-alias/modernize.sh render`, loads in Homebrew 7 (`Cask::CaskLoader::FromContentLoader`) with a `PostflightSteps` artifact. The currently published cask fails the same load with the error above. - Reinserting the `result = system_command ...` line into the rendered cask makes the render check fail.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
modernize.sh renderconverts only lines that start withsystem_command. A call anywhere else on a line, such asresult = system_command ..., was left inside the*_stepsblock, and Homebrew rejects the whole cask (undefined method 'system_command' for an instance of Homebrew::InstallSteps::DSL). The release then publishes an uninstallable cask thatbrew outdatedsilently skips; this is what happened tocodereview-cli.rendernow fails and prints the offending lines when any non-commentsystem_commandremains after the rewrite, so the release stops before the tap is updated.Validation
bash actions/homebrew-alias/test_modernize.sh: all pass, including a new case for an assignedsystem_command.homebrew-taprenders unchanged exceptcodereview-cli.rb, which is the broken one and is now rejected.The cask fix itself: open-cli-collective/codereview-cli#676