Skip to content

feat: install script to check for hard stops - #4489

Open
aldy505 wants to merge 6 commits into
masterfrom
aldy505/feat/check-hard-stop
Open

aldy505 wants to merge 6 commits into
masterfrom
aldy505/feat/check-hard-stop

Conversation

@aldy505

@aldy505 aldy505 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

The idea came from Discord, and Alex (stayalive) lay out a very good approach on this: https://discord.com/channels/621778831602221064/796028405833007104/1541789134006259814

Comment thread install/check-hard-stop.sh Outdated
Comment thread install/check-hard-stop.sh Outdated
Comment thread install/check-hard-stop.sh Outdated
Comment thread install/check-hard-stop.sh Outdated
Comment thread install/check-hard-stop.sh Outdated
Comment thread install/check-hard-stop.sh Outdated
Comment thread install/check-hard-stop.sh
Comment thread install/check-hard-stop.sh Outdated
Comment thread install/check-hard-stop.sh Outdated
@aldy505

aldy505 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

@aminvakil apparently, exit 0 terminates the install script. do you have any other suggestion for this? I want to avoid hadouken pattern.

EDIT; nevermind I just read this #4489 (comment), sorry about that

Comment thread install/check-hard-stop.sh Outdated
Comment thread install/check-hard-stop.sh Outdated
@BYK

BYK commented Aug 25, 2026

Copy link
Copy Markdown
Member

Just dropping this here: #3878 (review)

Overall, I think this is pretty good but we need a shared place and format to have the hard-stops so docs and the repo does not diverge. I'd propose JSON in a well-known location like the docs repo or this repo, and reading it with jq in the install script

@aldy505

aldy505 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Just dropping this here: #3878 (review)

Overall, I think this is pretty good but we need a shared place and format to have the hard-stops so docs and the repo does not diverge. I'd propose JSON in a well-known location like the docs repo or this repo, and reading it with jq in the install script

@BYK Yeah I remember about your comment. I don't know where the "shared place" should be. I don't want to have it on sentry, because I don't want to pull any image first, just for doing this. At the end of the day, I would think having these two separate would be good.

And yes, there will always be a maintenance burden for this one.

Comment thread install/check-hard-stop.sh
Comment thread install/check-hard-stop.sh
Comment thread install/check-hard-stop.sh
Comment thread install/check-hard-stop.sh
Comment thread install/check-hard-stop.sh Outdated
Comment thread install/check-hard-stop.sh
@BYK

BYK commented Aug 26, 2026

Copy link
Copy Markdown
Member

I don't want to have it on sentry, because I don't want to pull any image first, just for doing this

Not sure we are on the same page. My proposal is having this information in a separate, dedicated JSON file in this repo and then the docs repo fetching it at build time.

@aldy505

aldy505 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

I don't want to have it on sentry, because I don't want to pull any image first, just for doing this

Not sure we are on the same page. My proposal is having this information in a separate, dedicated JSON file in this repo and then the docs repo fetching it at build time.

Aaaaaahhhhhh, that makes sense. I dunno how to do it on the docs repo, but making a JSON file here would be doable.

Comment thread install/check-hard-stop.sh

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

if [[ -n "$current_version" ]]; then
# We iterate over the list of hard stops, and check whether the current
# version is below any of them.
local _wrote_version=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Top-level local crashes upgrade installs

High Severity

local _wrote_version=0 runs at script scope in a sourced file, which bash rejects. Combined with set -e in install.sh, any upgrade that already has a tracking file aborts immediately, so the hard-stop check never runs and the install cleanup trap can stop a live stack.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2b942d2. Configure here.

@aldy505
aldy505 added this pull request to stack #4507 September 13, 2026 15:09
@aldy505
aldy505 requested review from BYK and aminvakil September 13, 2026 15:25
@aldy505
aldy505 force-pushed the aldy505/feat/check-hard-stop branch from 9b35c7d to 2472e2c Compare September 13, 2026 15:26
if ((arr1[0] > arr2[0])); then
return 1
elif ((arr1[0] < arr2[0])); then
return -1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The compare_calver function's result is captured from stdout, but it returns a value via exit code. This causes compare_result to be empty, leading to an upgrade script failure.
Severity: CRITICAL

Suggested Fix

Modify the compare_calver function to echo its result to standard output instead of using return. Alternatively, change the calling code to capture the exit code using $? immediately after the function call: compare_calver "..." "..."; compare_result=$?. If capturing the exit code, be aware that return -1 becomes exit code 255 in bash, so the function's return values and the caller's checks must be adjusted to use valid exit codes (0-255).

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: install/check-hard-stop.sh#L74

Potential issue: The `compare_calver` function is designed to communicate its result via
an exit code (`return`), but the calling code captures its standard output into the
`compare_result` variable. Since the function does not write its result to stdout,
`compare_result` is always an empty string. This causes the subsequent conditional
checks to fail, leading the script to always execute the `else` block and exit with an
error. This bug triggers during the common upgrade scenario for existing installations,
causing the install script to fail.

Also affects:

  • install/check-hard-stop.sh:80~80
  • install/check-hard-stop.sh:86~86
  • install/check-hard-stop.sh:160~161

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2472e2c. Configure here.

fi

return 0 # Equal
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Compare result never reaches caller

High Severity

compare_calver reports its result via return, but the caller stores stdout in compare_result. The function never prints that value, so compare_result is always empty and every upgrade aborts on the unexpected-value path. return -1 is also not a valid bash status.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2472e2c. Configure here.

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

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants