Feat: Install from the newest release, with --ref to override - #874
Closed
huang195 wants to merge 2 commits into
Closed
Feat: Install from the newest release, with --ref to override#874huang195 wants to merge 2 commits into
huang195 wants to merge 2 commits into
Conversation
The installer refuses every install against every published release:
error: checksums.txt has no entry for abctl_v0.7.0-alpha.3_darwin_arm64.tar.gz
— refusing to install it unverified
The entry is there. The release workflow generates checksums with
`sha256sum ./*.tar.gz`, so every line reads "HASH ./abctl_....tar.gz", and
the pattern I introduced in 79f2f57 — "[[:space:]]\*?NAME$" — requires the
name immediately after whitespace or a binary-mode asterisk. A "./" in
between means nothing matches.
That commit fixed a fail-OPEN (an alternation succeeded on one of two
archives, so a partial checksums.txt installed the other unverified) and
replaced it with a fail-CLOSED that blocks everyone. The fail-closed is the
safer direction of the two, but it is still a bug, and it is worse in
practice: nobody can install at all.
The pattern now accepts the name preceded by start-of-line, whitespace, "*",
or "/", which covers "./name", "dist/name", "*name" and a bare "name".
Tested against the real published checksums.txt for v0.7.0-alpha.3 plus six
constructed cases: no prefix, binary mode, a nested path, partial coverage
(the fail-open this guard exists for — still caught), a decoy where the name
appears mid-line, and a suffix impostor "xyzabctl_....tar.gz". All seven
behave. End to end, the exact failing command now verifies both archives
("./abctl_...: OK", "./authbridge-proxy_...: OK") and installs both binaries.
The lesson worth recording: the first version of this guard was never run
against a real checksums.txt, only against fixtures I wrote from the same
mistaken assumption about the format.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
The documented command fetches install.sh from main and runs it. main is
whatever landed last, so a `curl | sh` executes unreviewed and unreleased
changes on someone's laptop the moment they merge — which is exactly how a
broken checksum pattern of mine reached a user and blocked every install.
The script now re-runs the copy from the newest release and hands it the same
arguments. Releases are tested; main is not. Two escape hatches:
--ref=main run this copy, unreleased changes included
--ref=vX.Y.Z pin the installer to a release
AUTHBRIDGE_REF is the environment equivalent. When the script came from a
release tag, the binaries default to that same tag, so the script and the
binaries it installs are one tested set rather than two independently-moving
things; AUTHBRIDGE_VERSION still overrides.
Details that took a test to get right:
- --ref is stripped before re-exec. A released script from before --ref
existed rejects it as an unknown option, which is exactly what happened
on the first run of this.
- The argument list is rebuilt by rotating the positional parameters rather
than building a string, so an argument containing a space survives.
- AUTHBRIDGE_SCRIPT_REF is both the ref name and the recursion guard: the
child sees it set and does not bootstrap again. Verified the bootstrap
line appears exactly once.
- If the resolved ref has no authbridge/install.sh, it warns and continues
with the current copy. That is not hypothetical: the newest release today
is v0.7.0-alpha.3, which predates the rename from install-demo.sh, so the
fallback is the live path until the next release exists.
Tested: the default (falls back with a warning today), --ref=main, --ref with
a commit SHA that does have the script, AUTHBRIDGE_REF, the recursion guard,
argument propagation, version pinning, and --help through the documented pipe.
The SHA test is the one worth naming: the parent had the checksum fix and the
child did not, and the child failed on the checksum bug — which is direct
evidence the re-exec runs the pinned copy's code rather than the parent's.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
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. Comment |
Member
Author
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
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
2 participants
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.
Stacked on #871 — review/merge that first.
Why
The documented command fetches
install.shfrommainand runs it.mainiswhatever landed last, so a
curl | shexecutes unreviewed, unreleased changes themoment they merge. That is not theoretical: a broken checksum pattern of mine
reached a user this way and blocked every install (#871).
What changes
The script re-runs the copy from the newest release, passing the same
arguments. Releases are tested;
mainis not.AUTHBRIDGE_REFis the env equivalent. When the script came from a release tag,the binaries default to that same tag, so the script and the binaries it
installs are one tested set.
AUTHBRIDGE_VERSIONstill overrides.Details that needed a test to get right
--refis stripped before re-exec. A released script from before--refexisted rejects it as an unknown option — which is what happened on my first run.
building a string, so an argument containing a space survives.
AUTHBRIDGE_SCRIPT_REFis both the ref name and the recursion guard — thechild sees it set and does not bootstrap again. Verified it fires exactly once.
authbridge/install.sh: warn and continue withthe current copy. Not hypothetical — the newest release today is
v0.7.0-alpha.3, which predates the rename frominstall-demo.sh, so thefallback is the live path until the next release exists.
Ordering
This is safe to merge now because of that fallback, but it only becomes fully
effective once a release exists whose tree contains
authbridge/install.sh— i.e.the next tag cut from
main.Tested
Default (warns and falls back today),
--ref=main,--ref=<sha>against a refthat does have the script,
AUTHBRIDGE_REF, the recursion guard, argumentpropagation, version pinning, and
--helpthrough the documented pipe.The SHA test is worth naming: the parent had #871's checksum fix, the child did
not, and the child failed on the checksum bug — direct evidence that the re-exec
runs the pinned copy's code rather than the parent's.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com