chore(makefile): refuse make install in non-interactive mode#721
Open
liangshuo-1 wants to merge 1 commit intomainfrom
Open
chore(makefile): refuse make install in non-interactive mode#721liangshuo-1 wants to merge 1 commit intomainfrom
make install in non-interactive mode#721liangshuo-1 wants to merge 1 commit intomainfrom
Conversation
`make install` builds from source and produces a binary that won't match
official release artifacts (different version metadata, no codesigning,
breaks the self-update flow). When AI assistants see "new version
available" they tend to pattern-match to `make install` and silently
clobber the user's properly-installed binary.
Add a TTY gate so `make install`:
- Refuses outright in non-interactive mode (AI Bash tool, CI, pipes)
with a message pointing at `lark-cli update`.
- Prompts y/N when run from a real terminal (contributor flow).
- Honors `I_AM_A_CONTRIBUTOR=1` as an escape hatch for scripted
contributor workflows.
Also adds a banner comment at the top of the Makefile so anyone
(including AI) reading the file sees the intended audience first.
Change-Id: I17faf6ac34b571f7c59128a87e85ed19dd581e96
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
github-actions
Bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
35-35: Consider gating before build to avoid unnecessary compilation on refusal.With
install: build, non-interactive runs still build even when install is blocked. Movingbuildafter the gate would fail faster for AI/CI/scripted misuse.Refactor option
-install: build +install: `@if` [ "$$I_AM_A_CONTRIBUTOR" = "1" ]; then \ : ; \ elif [ -t 0 ] && [ -t 1 ]; then \ echo "" ; \ echo " make install builds from source — for CONTRIBUTORS only." ; \ echo " To upgrade an existing lark-cli, run: lark-cli update" ; \ echo "" ; \ printf " Continue installing from source? (y/N) " ; \ read ans ; \ case "$$ans" in y|Y|yes|YES) ;; *) echo "Aborted." ; exit 1 ;; esac ; \ else \ echo "make install: refusing in non-interactive mode." ; \ echo " To upgrade lark-cli: run \`lark-cli update\`." ; \ echo " To install from source non-interactively: I_AM_A_CONTRIBUTOR=1 make install" ; \ exit 1 ; \ fi + @$(MAKE) build install -d $(PREFIX)/bin install -m755 $(BINARY) $(PREFIX)/bin/$(BINARY)Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 35, The install target currently depends on build ("install: build") causing a full build even when the install gate fails; change the Makefile so the install recipe first performs the gate/check step and only invokes the build target after the gate succeeds (i.e., move or reorder the dependency/commands so the gate runs before calling the build target), and apply the same reordering for the similar targets around lines with the other install-like rules referenced (the other occurrences similar to "install" at the later block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 36-37: The contributor bypass condition in the Makefile currently
triggers for any non-empty I_AM_A_CONTRIBUTOR; change the conditional to only
succeed when I_AM_A_CONTRIBUTOR is exactly "1". Locate the conditional that
references I_AM_A_CONTRIBUTOR (the line starting with `@if` [ -n
"$$I_AM_A_CONTRIBUTOR" ]; then) and replace the test with an exact string
comparison against "1" (so the block only runs when I_AM_A_CONTRIBUTOR=1),
preserving Makefile variable escaping and the existing then/else block
structure.
---
Nitpick comments:
In `@Makefile`:
- Line 35: The install target currently depends on build ("install: build")
causing a full build even when the install gate fails; change the Makefile so
the install recipe first performs the gate/check step and only invokes the build
target after the gate succeeds (i.e., move or reorder the dependency/commands so
the gate runs before calling the build target), and apply the same reordering
for the similar targets around lines with the other install-like rules
referenced (the other occurrences similar to "install" at the later block).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+36
to
+37
| @if [ -n "$$I_AM_A_CONTRIBUTOR" ]; then \ | ||
| : ; \ |
There was a problem hiding this comment.
Tighten contributor bypass to the documented =1 value.
Line 36 currently bypasses on any non-empty I_AM_A_CONTRIBUTOR value, which is looser than the documented contract (I_AM_A_CONTRIBUTOR=1).
Suggested patch
- `@if` [ -n "$$I_AM_A_CONTRIBUTOR" ]; then \
+ `@if` [ "$$I_AM_A_CONTRIBUTOR" = "1" ]; then \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @if [ -n "$$I_AM_A_CONTRIBUTOR" ]; then \ | |
| : ; \ | |
| `@if` [ "$$I_AM_A_CONTRIBUTOR" = "1" ]; then \ | |
| : ; \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 36 - 37, The contributor bypass condition in the
Makefile currently triggers for any non-empty I_AM_A_CONTRIBUTOR; change the
conditional to only succeed when I_AM_A_CONTRIBUTOR is exactly "1". Locate the
conditional that references I_AM_A_CONTRIBUTOR (the line starting with `@if` [ -n
"$$I_AM_A_CONTRIBUTOR" ]; then) and replace the test with an exact string
comparison against "1" (so the block only runs when I_AM_A_CONTRIBUTOR=1),
preserving Makefile variable escaping and the existing then/else block
structure.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
=======================================
Coverage 64.17% 64.17%
=======================================
Files 504 504
Lines 44287 44287
=======================================
Hits 28420 28420
Misses 13393 13393
Partials 2474 2474 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f985b49ff8323c6ce05136c1d3cc9b5623a21326🧩 Skill updatenpx skills add larksuite/cli#chore/makefile-install-gate -y -g |
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
1 participant
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.
Why
make installbuilds from source and produces a binary that won't match official release artifacts (different version metadata, no codesigning, breaks the self-update flow). When the CLI shows a "new version available" notice, AI coding assistants tend to pattern-match tomake installand silently clobber the user's properly-installed binary — a real scenario we saw recently with Claude Code.What
Add a TTY gate to the
installtarget:lark-cli update.y/Nconfirmation, then proceeds.I_AM_A_CONTRIBUTOR=1 make install→ escape hatch for scripted contributor workflows.Also adds a banner comment at the top of the Makefile so anyone (including AI) reading the file sees the intended audience first.
Test plan
echo "" | make install(simulating non-TTY) → refuses with exit 1, printslark-cli updatehint.I_AM_A_CONTRIBUTOR=1 make install→ bypasses gate.make installin a real terminal → prompts y/N;yproceeds, anything else aborts.Follow-ups (separate PRs)
_notice.update.commandfield so the suggested command is explicit.Summary by CodeRabbit