fix(install): make checksum verification mandatory#1
fix(install): make checksum verification mandatory#1areporeporepo wants to merge 3 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThe installer script now enforces mandatory checksum verification, failing immediately when checksums cannot be downloaded or verification tools are unavailable. An explicit opt-out mechanism via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security and robustness of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fallback mechanism for OpenAI chat completion backend validation, allowing the system to first attempt validation with max_completion_tokens and then retry with max_tokens if the initial request fails with a 400 error. This is achieved by adding a fallback_body to the ValidationProbe struct and refactoring the verify_backend_endpoint function, along with new unit tests to cover these scenarios. Additionally, the Landlock sandbox application now includes more detailed logging, promoting a debug message to a warning when the sandbox is unavailable in BestEffort compatibility mode. The install.sh script has also been updated to allow skipping checksum verification via an environment variable or CLI flag, and it now enforces stricter checksum verification by default, failing if checksums are missing or verification tools are not found. No feedback was provided in the review comments.
Greptile SummaryThis PR hardens the install script by making checksum verification mandatory — previously both a missing Key changes:
Remaining observations:
Confidence Score: 4/5Safe to merge with one small logic gap: the awk checksum lookup doesn't handle binary-mode entries with a The two prior P1 findings (OPENSHELL_NO_VERIFY silently ignoring non-"1" values; grep treating filenames as regexes) are both fixed. The remaining findings are P2: a documentation inconsistency where the help text still says "Set to '1'" after the truthy-value expansion, and the awk binary-mode edge case. Score stays at 4 rather than 5 because of the latent correctness gap in checksum lookup. install.sh — the awk checksum field match on line 189 and documentation on lines 16 and 60. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[main starts] --> B[Normalise OPENSHELL_NO_VERIFY\n1/true/yes/y → _skip_checksum=1\nelse → _skip_checksum=0]
B --> C[Parse CLI flags]
C --> D{--no-verify-checksum?}
D -- yes --> E[_skip_checksum=1]
D -- no --> F[Download binary archive]
E --> F
F --> G{Download success?}
G -- no --> H[error: failed to download]
G -- yes --> I{_skip_checksum == 1?}
I -- yes --> J[warn: checksum verification skipped]
I -- no --> K[Download checksums file]
K --> L{Download success?}
L -- no --> M[error: failed to download checksums file]
L -- yes --> N[awk: look up hash by filename]
N --> O{Hash found?}
O -- no --> P[error: no checksum found]
O -- yes --> Q{shasum or sha256sum available?}
Q -- no --> R[error: tool not found]
Q -- yes --> S[Run hash check]
S --> T{Match?}
T -- no --> U[error: checksum verification failed]
T -- yes --> V[Extract and install binary]
J --> V
Reviews (1): Last reviewed commit: "fix(install): accept common truthy value..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let (Err(err), Some(fallback_body)) = (&result, probe.fallback_body) { | ||
| if err.kind == ValidationFailureKind::RequestShape { | ||
| return try_validation_request( | ||
| client, | ||
| route, | ||
| probe.path, | ||
| probe.protocol, | ||
| headers, | ||
| fallback_body, | ||
| ) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
The fallback from max_completion_tokens to max_tokens is semantically meaningful only for an HTTP 400 "unsupported parameter" response. However, ValidationFailureKind::RequestShape is assigned to 400, 404, 405, and 422 (see lines 349–350 in try_validation_request). A 404 (endpoint not found) or 405 (method not allowed) means the entire path is wrong — changing the body parameter won't help — yet the code will fire an extra try_validation_request before finally returning an error.
This adds unnecessary latency and a spurious second request on every misconfigured endpoint. Consider narrowing the guard to specifically detect a 400, or introduce a dedicated variant like ValidationFailureKind::UnsupportedParameter set only on HTTP 400, and guard the fallback on that flag instead of the broader RequestShape kind.
There was a problem hiding this comment.
Pull request overview
Makes installation checksum verification mandatory by default (with explicit opt-out), and updates backend validation probing to handle OpenAI’s newer max_completion_tokens parameter while improving Landlock sandbox logging.
Changes:
install.sh: Fail hard when checksums tooling/entries or checksums download are missing; add--no-verify-checksum/OPENSHELL_NO_VERIFY=1opt-out and update usage text.openshell-router: Switch OpenAI chat validation probe tomax_completion_tokenswith a fallback retry usingmax_tokenson request-shape failures; add targeted tests.openshell-sandbox: Add higher-signal logs when applying Landlock and when Landlock is unavailable in best-effort mode.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| install.sh | Enforces checksum verification by default and adds explicit opt-out mechanisms. |
| crates/openshell-server/src/inference.rs | Updates test expectations to match the new OpenAI validation probe parameter. |
| crates/openshell-sandbox/src/sandbox/linux/landlock.rs | Adds additional info/warn logging around Landlock application and fallback behavior. |
| crates/openshell-router/src/backend.rs | Updates OpenAI chat validation probe payload + adds fallback retry logic and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
install.sh
Outdated
| error "checksum verification failed for ${_filename}" | ||
| fi | ||
| else | ||
| error "failed to download checksums file from ${_checksums_url} (set OPENSHELL_NO_VERIFY=1 to skip verification)" |
There was a problem hiding this comment.
This error message suggests only OPENSHELL_NO_VERIFY=1 as the opt-out. Since the script now supports --no-verify-checksum, it would be clearer to mention both opt-out mechanisms here too.
| error "failed to download checksums file from ${_checksums_url} (set OPENSHELL_NO_VERIFY=1 to skip verification)" | |
| error "failed to download checksums file from ${_checksums_url} (set OPENSHELL_NO_VERIFY=1 or use --no-verify-checksum to skip verification)" |
| /// Alternate body to try when the primary probe fails with HTTP 400. | ||
| /// Used for OpenAI chat completions where newer models require | ||
| /// `max_completion_tokens` while legacy/self-hosted backends only | ||
| /// accept `max_tokens`. | ||
| fallback_body: Option<bytes::Bytes>, |
There was a problem hiding this comment.
The doc comment says the fallback is only used when the primary probe fails with HTTP 400, but the retry logic is keyed off ValidationFailureKind::RequestShape (which can also be 404/405/422). Please update the wording to match the actual behavior (e.g., “request-shape error”) or tighten the retry condition if you truly only want to retry on 400.
| // If the primary probe failed with a request-shape error (HTTP 400) and | ||
| // there is a fallback body, retry with the alternate token parameter. | ||
| // This handles the split between `max_completion_tokens` (GPT-5+) and | ||
| // `max_tokens` (legacy/self-hosted backends). | ||
| if let (Err(err), Some(fallback_body)) = (&result, probe.fallback_body) { | ||
| if err.kind == ValidationFailureKind::RequestShape { | ||
| return try_validation_request( |
There was a problem hiding this comment.
These comments say the fallback retry happens only for HTTP 400, but the code retries whenever err.kind == ValidationFailureKind::RequestShape (which is also used for 404/405/422 in try_validation_request). Either adjust the comment to describe “request-shape” failures, or change the condition to only retry on 400 if that’s the intent.
| // Use max_completion_tokens (modern OpenAI parameter, required by GPT-5+) | ||
| // with max_tokens as fallback for legacy/self-hosted backends. | ||
| return Ok(ValidationProbe { | ||
| path: "/v1/chat/completions", | ||
| protocol: "openai_chat_completions", | ||
| body: bytes::Bytes::from_static( | ||
| br#"{"messages":[{"role":"user","content":"ping"}],"max_tokens":32}"#, | ||
| br#"{"messages":[{"role":"user","content":"ping"}],"max_completion_tokens":32}"#, | ||
| ), |
There was a problem hiding this comment.
The PR title/description are scoped to install.sh checksum enforcement, but this PR also changes router backend validation behavior (max_completion_tokens + fallback) and updates Landlock logging. Please update the PR description to reflect these additional behavioral changes, or split them into a separate PR so the install-script change can be reviewed/rolled back independently.
install.sh
Outdated
| else | ||
| warn "sha256sum/shasum not found, skipping checksum verification" | ||
| return 0 | ||
| error "sha256sum or shasum is required for checksum verification (install coreutils or set OPENSHELL_NO_VERIFY=1 to skip)" |
There was a problem hiding this comment.
The error text only points users at OPENSHELL_NO_VERIFY=1, but this script also supports --no-verify-checksum. Consider mentioning the CLI flag here as an equivalent opt-out so users running ./install.sh ... see the right remediation.
| error "sha256sum or shasum is required for checksum verification (install coreutils or set OPENSHELL_NO_VERIFY=1 to skip)" | |
| error "sha256sum or shasum is required for checksum verification (install coreutils, set OPENSHELL_NO_VERIFY=1, or pass --no-verify-checksum to skip)" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
install.sh (1)
189-201: Correctly makes verification mandatory with clear error messages.The conversion from
warn+return 0toerror(exit) aligns with the PR objective of mandatory verification. The error messages helpfully guide users toward the opt-out mechanism if needed.Optional hardening: Consider using
grep -Ffor fixed string matching to avoid regex interpretation of dots in the filename.,
🛡️ Optional: Use fixed-string grep
- _vc_expected="$(grep "$_vc_filename" "$_vc_checksums" | awk '{print $1}')" + _vc_expected="$(grep -F "$_vc_filename" "$_vc_checksums" | awk '{print $1}')"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 189 - 201, The checksum lookup using grep "$_vc_filename" in the assignment to _vc_expected can misinterpret regex metacharacters in filenames; change the grep invocation to fixed-string mode (grep -F) when matching $_vc_filename against $_vc_checksums, keeping the rest of the flow intact (variables _vc_expected, _vc_filename, _vc_checksums) and preserving the mandatory verification/error behavior via the has_cmd checks and error calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/openshell-router/src/backend.rs`:
- Around line 259-274: The current retry uses
ValidationFailureKind::RequestShape for all request-shape failures, which also
includes 404/405 path/method errors; update the conditional guarding the
fallback retry so it only retries when the failure is due to a request
body/parameter shape mismatch (not path/method errors). Concretely, in the block
that checks if let (Err(err), Some(fallback_body)) = (&result,
probe.fallback_body) { ... }, replace the plain err.kind ==
ValidationFailureKind::RequestShape check with a more specific predicate that
detects a body-shape failure (e.g., inspect an error field like err.status_code
to exclude 404/405 or use an err.shape_kind/is_body_shape helper) and only call
try_validation_request(client, route, probe.path, probe.protocol, headers,
fallback_body). Ensure you reference ValidationFailureKind::RequestShape,
try_validation_request, probe.fallback_body, probe.path and probe.protocol when
making the change.
- Around line 591-605: The test verify_non_chat_completions_no_fallback doesn't
assert the mock was called exactly once, so it can pass even if retries occur;
update the mock definition for mock_server (the Mock::given(...) chain that
responds_with(ResponseTemplate::new(400)...) ) to include an explicit
request-count assertion like expect(1) before mounting to ensure the
anthropic_messages probe is not retried and the test enforces a single request.
---
Nitpick comments:
In `@install.sh`:
- Around line 189-201: The checksum lookup using grep "$_vc_filename" in the
assignment to _vc_expected can misinterpret regex metacharacters in filenames;
change the grep invocation to fixed-string mode (grep -F) when matching
$_vc_filename against $_vc_checksums, keeping the rest of the flow intact
(variables _vc_expected, _vc_filename, _vc_checksums) and preserving the
mandatory verification/error behavior via the has_cmd checks and error calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15eca818-df29-449f-bfec-ab6126b171fc
📒 Files selected for processing (4)
crates/openshell-router/src/backend.rscrates/openshell-sandbox/src/sandbox/linux/landlock.rscrates/openshell-server/src/inference.rsinstall.sh
| // If the primary probe failed with a request-shape error (HTTP 400) and | ||
| // there is a fallback body, retry with the alternate token parameter. | ||
| // This handles the split between `max_completion_tokens` (GPT-5+) and | ||
| // `max_tokens` (legacy/self-hosted backends). | ||
| if let (Err(err), Some(fallback_body)) = (&result, probe.fallback_body) { | ||
| if err.kind == ValidationFailureKind::RequestShape { | ||
| return try_validation_request( | ||
| client, | ||
| route, | ||
| probe.path, | ||
| probe.protocol, | ||
| headers, | ||
| fallback_body, | ||
| ) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
Narrow the fallback retry to body-shape failures.
Line 264 keys off ValidationFailureKind::RequestShape, but try_validation_request() also classifies 404 and 405 that way. Switching max_completion_tokens to max_tokens cannot recover from a bad path or method, so misconfigured base URLs now incur a second POST before failing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/openshell-router/src/backend.rs` around lines 259 - 274, The current
retry uses ValidationFailureKind::RequestShape for all request-shape failures,
which also includes 404/405 path/method errors; update the conditional guarding
the fallback retry so it only retries when the failure is due to a request
body/parameter shape mismatch (not path/method errors). Concretely, in the block
that checks if let (Err(err), Some(fallback_body)) = (&result,
probe.fallback_body) { ... }, replace the plain err.kind ==
ValidationFailureKind::RequestShape check with a more specific predicate that
detects a body-shape failure (e.g., inspect an error field like err.status_code
to exclude 404/405 or use an err.shape_kind/is_body_shape helper) and only call
try_validation_request(client, route, probe.path, probe.protocol, headers,
fallback_body). Ensure you reference ValidationFailureKind::RequestShape,
try_validation_request, probe.fallback_body, probe.path and probe.protocol when
making the change.
| /// Non-chat-completions probes (e.g. anthropic_messages) should not | ||
| /// have a fallback — a 400 remains a hard failure. | ||
| #[tokio::test] | ||
| async fn verify_non_chat_completions_no_fallback() { | ||
| let mock_server = MockServer::start().await; | ||
| let route = test_route( | ||
| &mock_server.uri(), | ||
| &["anthropic_messages"], | ||
| AuthHeader::Custom("x-api-key"), | ||
| ); | ||
|
|
||
| Mock::given(method("POST")) | ||
| .and(path("/v1/messages")) | ||
| .respond_with(ResponseTemplate::new(400).set_body_string("bad request")) | ||
| .mount(&mock_server) |
There was a problem hiding this comment.
This test doesn't actually prove “no fallback”.
Without expect(1) on the mock, the test still passes if the anthropic probe is retried and receives another 400. Add an explicit request-count assertion so this locks in the behavior the docstring describes.
Possible tightening
Mock::given(method("POST"))
.and(path("/v1/messages"))
.respond_with(ResponseTemplate::new(400).set_body_string("bad request"))
+ .expect(1)
.mount(&mock_server)
.await;📝 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.
| /// Non-chat-completions probes (e.g. anthropic_messages) should not | |
| /// have a fallback — a 400 remains a hard failure. | |
| #[tokio::test] | |
| async fn verify_non_chat_completions_no_fallback() { | |
| let mock_server = MockServer::start().await; | |
| let route = test_route( | |
| &mock_server.uri(), | |
| &["anthropic_messages"], | |
| AuthHeader::Custom("x-api-key"), | |
| ); | |
| Mock::given(method("POST")) | |
| .and(path("/v1/messages")) | |
| .respond_with(ResponseTemplate::new(400).set_body_string("bad request")) | |
| .mount(&mock_server) | |
| /// Non-chat-completions probes (e.g. anthropic_messages) should not | |
| /// have a fallback — a 400 remains a hard failure. | |
| #[tokio::test] | |
| async fn verify_non_chat_completions_no_fallback() { | |
| let mock_server = MockServer::start().await; | |
| let route = test_route( | |
| &mock_server.uri(), | |
| &["anthropic_messages"], | |
| AuthHeader::Custom("x-api-key"), | |
| ); | |
| Mock::given(method("POST")) | |
| .and(path("/v1/messages")) | |
| .respond_with(ResponseTemplate::new(400).set_body_string("bad request")) | |
| .expect(1) | |
| .mount(&mock_server) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/openshell-router/src/backend.rs` around lines 591 - 605, The test
verify_non_chat_completions_no_fallback doesn't assert the mock was called
exactly once, so it can pass even if retries occur; update the mock definition
for mock_server (the Mock::given(...) chain that
responds_with(ResponseTemplate::new(400)...) ) to include an explicit
request-count assertion like expect(1) before mounting to ensure the
anthropic_messages probe is not retried and the test enforces a single request.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
install.sh (1)
267-278: Consider accepting additional truthy values forOPENSHELL_NO_VERIFY.Currently only
"1"skips verification. Users might intuitively tryOPENSHELL_NO_VERIFY=trueorOPENSHELL_NO_VERIFY=yes, which would silently not skip (since they don't equal"1"), potentially causing confusion when verification still fails.🔧 Optional: Accept common truthy values
- if [ "$_skip_checksum" = "1" ]; then + case "$_skip_checksum" in + 1|true|yes|TRUE|YES) + warn "checksum verification skipped (OPENSHELL_NO_VERIFY=1 or --no-verify-checksum)" + ;; + *) + info "verifying checksum..." + if download "$_checksums_url" "${_tmpdir}/checksums.txt"; then + if ! verify_checksum "${_tmpdir}/${_filename}" "${_tmpdir}/checksums.txt" "$_filename"; then + error "checksum verification failed for ${_filename}" + fi + else + error "failed to download checksums file from ${_checksums_url} (set OPENSHELL_NO_VERIFY=1 or use --no-verify-checksum to skip verification)" + fi + ;; + esac - warn "checksum verification skipped (OPENSHELL_NO_VERIFY=1 or --no-verify-checksum)" - else - info "verifying checksum..." - if download "$_checksums_url" "${_tmpdir}/checksums.txt"; then - if ! verify_checksum "${_tmpdir}/${_filename}" "${_tmpdir}/checksums.txt" "$_filename"; then - error "checksum verification failed for ${_filename}" - fi - else - error "failed to download checksums file from ${_checksums_url} (set OPENSHELL_NO_VERIFY=1 or use --no-verify-checksum to skip verification)" - fi - fiAlternatively, keep strict
"1"matching but update documentation to explicitly state only"1"is accepted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 267 - 278, The current check in the install.sh branch uses exact equality to "1" for _skip_checksum/OPENSHELL_NO_VERIFY; update the condition in the install script so it accepts common truthy values (e.g., "1", "true", "yes", "y") case-insensitively instead of only "1". Locate the conditional that tests _skip_checksum / OPENSHELL_NO_VERIFY in install.sh (the if [ "$_skip_checksum" = "1" ] block) and replace it with a check that normalizes/lowercases the value and matches against the allowed truthy set (or use a regex like ^(1|true|yes|y)$) so users can set OPENSHELL_NO_VERIFY=true or yes to skip checksum verification. Ensure behavior remains unchanged for falsy/absent values.e2e/install/sh_test.sh (1)
71-92: Minor: Temp directory not cleaned up after test.The
_skip_dircreated viamktemp -dis not removed after the test completes. While this is minor for e2e tests (OS cleans/tmpeventually), consider adding cleanup for consistency with other test patterns or if tests run frequently in CI.🧹 Optional cleanup addition
test_skip_checksum_env() { printf 'TEST: OPENSHELL_NO_VERIFY=1 skips checksum verification\n' - _skip_dir="$(mktemp -d)/bin" + _skip_base="$(mktemp -d)" + _skip_dir="${_skip_base}/bin" _skip_output="$(OPENSHELL_NO_VERIFY=1 \ OPENSHELL_INSTALL_DIR="$_skip_dir" \ SHELL="/bin/sh" \ PATH="/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin" \ sh "$INSTALL_SCRIPT" 2>&1)" || { fail "install succeeds with OPENSHELL_NO_VERIFY=1" "exit code: $?" + rm -rf "$_skip_base" return 1 } assert_output_contains "$_skip_output" "checksum verification skipped" \ "shows checksum skip message" if [ -f "$_skip_dir/openshell" ]; then pass "binary installed with checksum skip" else fail "binary installed with checksum skip" "not found at $_skip_dir/openshell" fi + rm -rf "$_skip_base" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/install/sh_test.sh` around lines 71 - 92, The test_skip_checksum_env function creates a temporary directory in _skip_dir via mktemp -d but never removes it; update test_skip_checksum_env to clean the temp directory after the test finishes (and on errors) by removing _skip_dir (e.g., rm -rf "$_skip_dir") or register a trap to delete _skip_dir on EXIT so the temp dir is removed whether the test passes or fails; keep the cleanup adjacent to the existing asserts and use the same _skip_dir variable name to locate the removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/install/sh_test.sh`:
- Around line 71-92: The test_skip_checksum_env function creates a temporary
directory in _skip_dir via mktemp -d but never removes it; update
test_skip_checksum_env to clean the temp directory after the test finishes (and
on errors) by removing _skip_dir (e.g., rm -rf "$_skip_dir") or register a trap
to delete _skip_dir on EXIT so the temp dir is removed whether the test passes
or fails; keep the cleanup adjacent to the existing asserts and use the same
_skip_dir variable name to locate the removal.
In `@install.sh`:
- Around line 267-278: The current check in the install.sh branch uses exact
equality to "1" for _skip_checksum/OPENSHELL_NO_VERIFY; update the condition in
the install script so it accepts common truthy values (e.g., "1", "true", "yes",
"y") case-insensitively instead of only "1". Locate the conditional that tests
_skip_checksum / OPENSHELL_NO_VERIFY in install.sh (the if [ "$_skip_checksum" =
"1" ] block) and replace it with a check that normalizes/lowercases the value
and matches against the allowed truthy set (or use a regex like
^(1|true|yes|y)$) so users can set OPENSHELL_NO_VERIFY=true or yes to skip
checksum verification. Ensure behavior remains unchanged for falsy/absent
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e7e072f-3225-494a-aa08-f01de04df652
📒 Files selected for processing (2)
e2e/install/sh_test.shinstall.sh
|
@coderabbitai review |
|
@gemini-code-assist review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a fallback mechanism for OpenAI chat completion probes in the router to accommodate differences between max_completion_tokens (for newer models) and max_tokens (for legacy/self-hosted backends). It adds a fallback_body field to ValidationProbe and modifies the validation logic to retry with the fallback if the primary probe fails with an HTTP 400 error. New tests have been added to cover this behavior. Additionally, the Landlock sandbox application now includes more informative logging (info/warn) regarding its status and compatibility. The install.sh script has been enhanced to allow skipping checksum verification via an OPENSHELL_NO_VERIFY environment variable or a --no-verify-checksum CLI flag, with stricter failure conditions if checksums or required tools are missing. A new end-to-end test verifies this skip functionality. There is no feedback to provide.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Skip checksum verification (not recommended) | ||
| curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/main/install.sh | OPENSHELL_NO_VERIFY=1 sh |
There was a problem hiding this comment.
usage() documents the --no-verify-checksum flag in OPTIONS, but the EXAMPLES section only shows opting out via OPENSHELL_NO_VERIFY. Consider adding an example invocation that uses --no-verify-checksum as well, so users discover the CLI form without reading the whole help text.
| # Skip checksum verification (not recommended) | |
| curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/main/install.sh | OPENSHELL_NO_VERIFY=1 sh | |
| # Skip checksum verification via environment variable (not recommended) | |
| curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/main/install.sh | OPENSHELL_NO_VERIFY=1 sh | |
| # Skip checksum verification via CLI flag (not recommended) | |
| ./install.sh --no-verify-checksum |
| case "$arg" in | ||
| --help) | ||
| usage |
There was a problem hiding this comment.
The new --no-verify-checksum flag is introduced here, but the e2e install tests only cover the OPENSHELL_NO_VERIFY=1 path. Adding an e2e case for the CLI flag would help prevent regressions in argument parsing and precedence vs the env var.
| // If the primary probe failed with a request-shape error (HTTP 400) and | ||
| // there is a fallback body, retry with the alternate token parameter. | ||
| // This handles the split between `max_completion_tokens` (GPT-5+) and | ||
| // `max_tokens` (legacy/self-hosted backends). | ||
| if let (Err(err), Some(fallback_body)) = (&result, probe.fallback_body) { | ||
| if err.kind == ValidationFailureKind::RequestShape { | ||
| return try_validation_request( | ||
| client, | ||
| route, | ||
| probe.path, | ||
| probe.protocol, | ||
| headers, | ||
| fallback_body, | ||
| ) | ||
| .await; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The retry condition uses err.kind == ValidationFailureKind::RequestShape, but try_validation_request() classifies 400/404/405/422 as RequestShape. That means this fallback will also retry on 404/405 (wrong path/method), where changing the token parameter can’t help and just adds an extra request. Consider carrying the HTTP status through ValidationFailure (or returning it alongside the failure) so the fallback can be limited to statuses that actually indicate a schema/parameter mismatch (e.g. 400/422).
| // If the primary probe failed with a request-shape error (HTTP 400) and | |
| // there is a fallback body, retry with the alternate token parameter. | |
| // This handles the split between `max_completion_tokens` (GPT-5+) and | |
| // `max_tokens` (legacy/self-hosted backends). | |
| if let (Err(err), Some(fallback_body)) = (&result, probe.fallback_body) { | |
| if err.kind == ValidationFailureKind::RequestShape { | |
| return try_validation_request( | |
| client, | |
| route, | |
| probe.path, | |
| probe.protocol, | |
| headers, | |
| fallback_body, | |
| ) | |
| .await; | |
| } | |
| } |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
install.sh (2)
16-17: Keep the help text aligned with the accepted env values.
main()accepts1/true/yes/y, but the inline docs and usage only advertise"1". Updating the wording here would keep the documented contract in sync with the implementation.📝 Suggested wording
-# OPENSHELL_NO_VERIFY - Set to "1" to skip checksum verification (not recommended) +# OPENSHELL_NO_VERIFY - Set to 1/true/yes/y to skip checksum verification (not recommended) ... - OPENSHELL_NO_VERIFY Set to "1" to skip checksum verification (not recommended) + OPENSHELL_NO_VERIFY Set to 1/true/yes/y to skip checksum verification (not recommended)Also applies to: 54-60, 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 16 - 17, Update the help/usage text for OPENSHELL_NO_VERIFY to match the values accepted by main() by listing "1/true/yes/y" (not just "1"); locate the usage/help strings that mention OPENSHELL_NO_VERIFY (the inline docs and usage output) and change their wording to state the full accepted set ("1/true/yes/y") and/or equivalent case-insensitive true values so the documented contract matches the main() parsing logic; apply the same wording change to the other help occurrences that reference OPENSHELL_NO_VERIFY in the script.
272-282: Add regression coverage for the new hard-fail branches.This block is now the core security boundary. A focused test for checksum-file download failure, plus one for a verification failure path, would make it much harder to regress back to warning-and-continue behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 272 - 282, Add regression tests that exercise the hard-fail branches in the checksum verification block: one test should simulate download "$_checksums_url" failing (e.g., mock the download function or the network call) and assert the script invokes error and exits non-zero, and a second test should simulate verify_checksum returning false for "${_tmpdir}/${_filename}" with "${_tmpdir}/checksums.txt" and assert error is invoked and the script exits non-zero; target the logic that checks $_skip_checksum, calls download and verify_checksum and uses error/warn/info so your tests mock/stub download and verify_checksum and verify the error path is taken rather than allowing a warning-and-continue behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@install.sh`:
- Around line 16-17: Update the help/usage text for OPENSHELL_NO_VERIFY to match
the values accepted by main() by listing "1/true/yes/y" (not just "1"); locate
the usage/help strings that mention OPENSHELL_NO_VERIFY (the inline docs and
usage output) and change their wording to state the full accepted set
("1/true/yes/y") and/or equivalent case-insensitive true values so the
documented contract matches the main() parsing logic; apply the same wording
change to the other help occurrences that reference OPENSHELL_NO_VERIFY in the
script.
- Around line 272-282: Add regression tests that exercise the hard-fail branches
in the checksum verification block: one test should simulate download
"$_checksums_url" failing (e.g., mock the download function or the network call)
and assert the script invokes error and exits non-zero, and a second test should
simulate verify_checksum returning false for "${_tmpdir}/${_filename}" with
"${_tmpdir}/checksums.txt" and assert error is invoked and the script exits
non-zero; target the logic that checks $_skip_checksum, calls download and
verify_checksum and uses error/warn/info so your tests mock/stub download and
verify_checksum and verify the error path is taken rather than allowing a
warning-and-continue behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d7c7bcd-2995-45f6-9369-b4eba7a70727
📒 Files selected for processing (2)
e2e/install/sh_test.shinstall.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/install/sh_test.sh
- verify_checksum() now errors instead of warning when sha256sum/shasum is unavailable or when no checksum entry exists for the archive - main() now errors when the checksums file cannot be downloaded - Add --no-verify-checksum flag and OPENSHELL_NO_VERIFY env var for explicit opt-out in environments where verification is not feasible - Update usage() with new flag, env var, and example Previously, an attacker who could manipulate the download (MITM without HTTPS pinning) could serve a binary without the checksum file, and the installer would proceed silently. Now the installer fails loudly unless the operator explicitly opts out. Closes NVIDIA#590 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: areporeporepo <areporeporepo@users.noreply.github.com>
- Use awk exact-field match instead of grep regex for checksum lookup - Include both opt-out mechanisms (env var + flag) in all error messages - Add e2e test for OPENSHELL_NO_VERIFY=1 checksum skip path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: areporeporepo <areporeporepo@users.noreply.github.com>
- Accept 1, true, yes, y (case-insensitive) for OPENSHELL_NO_VERIFY - Clean up temp directory in e2e checksum skip test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: areporeporepo <areporeporepo@users.noreply.github.com>
areporeporepo
force-pushed
the
fix/install-mandatory-checksum
branch
from
7cbf39f to
4336c0c
Compare
| # Environment variables: | ||
| # OPENSHELL_VERSION - Release tag to install (default: latest tagged release) | ||
| # OPENSHELL_INSTALL_DIR - Directory to install into (default: ~/.local/bin) | ||
| # OPENSHELL_NO_VERIFY - Set to "1" to skip checksum verification (not recommended) |
There was a problem hiding this comment.
The header comment on line 16 says Set to "1" and the usage() help text on line 60 repeats the same phrasing:
OPENSHELL_NO_VERIFY Set to "1" to skip checksum verification (not recommended)
However, main() now normalises the variable to accept 1, true, yes, and y (case-insensitive). Both lines (16 and 60) should be updated to list the full set of accepted truthy values.
| # OPENSHELL_NO_VERIFY - Set to "1" to skip checksum verification (not recommended) | |
| # OPENSHELL_NO_VERIFY - Set to "1", "true", "yes", or "y" to skip checksum verification (not recommended) |
| _vc_filename="$3" | ||
|
|
||
| _vc_expected="$(grep "$_vc_filename" "$_vc_checksums" | awk '{print $1}')" | ||
| _vc_expected="$(awk -v fname="$_vc_filename" '$2 == fname { print $1 }' "$_vc_checksums")" |
There was a problem hiding this comment.
awk '$2 == fname' silently misses binary-mode checksum entries
The awk approach is a solid fix for the previous grep regex-interpretation concern, but it introduces a stricter format requirement. When sha256sum generates a checksums file in binary mode the line looks like:
abc123... *openshell-aarch64-unknown-linux-musl.tar.gz
awk's default field split gives $1 = abc123..., $2 = *openshell-... — with the leading asterisk — so $2 == fname never matches the plain filename, and the script exits with "no checksum found" even though a valid entry is present.
A field-aware approach that strips a leading * would be robust to both text-mode and binary-mode:
_vc_expected="$(awk -v fname="$_vc_filename" '{ f=$2; sub(/^\*/, "", f); if (f == fname) print $1 }' "$_vc_checksums")"This retains exact-filename matching (no regex) while tolerating the binary-mode asterisk prefix.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
install.sh (1)
16-16: Document all acceptedOPENSHELL_NO_VERIFYtruthy values.Line 60 says “set to
1”, but Lines 232-234 also accepttrue/yes/y. Consider documenting the full accepted set to match runtime behavior.💡 Suggested doc tweak
-# OPENSHELL_NO_VERIFY - Set to "1" to skip checksum verification (not recommended) +# OPENSHELL_NO_VERIFY - Set to "1", "true", "yes", or "y" to skip checksum verification (not recommended) ... - OPENSHELL_NO_VERIFY Set to "1" to skip checksum verification (not recommended) + OPENSHELL_NO_VERIFY Set to "1"/"true"/"yes"/"y" to skip checksum verification (not recommended)Also applies to: 60-60, 230-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` at line 16, Update the install.sh comment for OPENSHELL_NO_VERIFY to list all accepted truthy values (not just "1") so the docs match runtime behavior: mention that the installer treats "1", "true", "yes", and "y" (case-insensitive) as truthy; reference OPENSHELL_NO_VERIFY in the comment near the top of the script and ensure the same wording is used for the help/usage lines around where OPENSHELL_NO_VERIFY is parsed (the block that checks the variable at runtime).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@install.sh`:
- Line 16: Update the install.sh comment for OPENSHELL_NO_VERIFY to list all
accepted truthy values (not just "1") so the docs match runtime behavior:
mention that the installer treats "1", "true", "yes", and "y" (case-insensitive)
as truthy; reference OPENSHELL_NO_VERIFY in the comment near the top of the
script and ensure the same wording is used for the help/usage lines around where
OPENSHELL_NO_VERIFY is parsed (the block that checks the variable at runtime).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3bd85ec-a2ea-4b89-83a4-a06d554773a0
📒 Files selected for processing (2)
e2e/install/sh_test.shinstall.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/install/sh_test.sh
Summary
verify_checksum()now errors instead of warning whensha256sum/shasumis unavailable or when no checksum entry exists for the archivemain()now errors when the checksums file cannot be downloaded--no-verify-checksumflag andOPENSHELL_NO_VERIFYenv var for explicit opt-outusage()with new flag, env var, and exampleRelated Issue
Closes NVIDIA#590
Testing
shellcheck install.sh— passes (zero warnings)sh -n install.sh— syntax valide2e/install/{sh,bash,zsh}_test.sh) unaffected — they use real releases with checksumsChecklist
Summary by CodeRabbit
New Features
Tests