feat: ship verified self-update and artifact alignment - #165
Conversation
|
@codex review |
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSatelle adds verified self-update execution, installation ownership checks, transactional replacement, and remote Host handoff. Host updates preserve explicit versions. npm launchers pass package context. Release validation and publication enforce the six-target platform matrix. ChangesSelf-update contracts and engine
npm installation ownership propagation
Release matrix and publication
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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. Comment |
![greptile-apps[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aae920ce82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
npm/satelle/lib/launcher.cjs (1)
305-331: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnguarded
realpathSynccan crash the launcher with a raw stack trace.Lines 318 and 327 call
realpathSync(launcherPath)outside anytry.mainrethrows anything that is not aLauncherError, so anENOENTorEPERMhere produces an unhandled exception instead of thesatelle: <code>: <message>contract. The same value is also computed twice.Hoist it once and treat failure as "no context", which matches the surrounding fail-closed style.
♻️ Proposed refactor
if (!packageName || !launcherPath) { return undefined; } + let canonicalLauncherPath; + try { + canonicalLauncherPath = realpathSync(launcherPath); + } catch { + return undefined; + } const candidates = globalOwners.map((owner) => ({ manager: owner.manager, scope: "global", package_name: packageName, install_root: path.resolve(owner.installRoot), - launcher_path: realpathSync(launcherPath), + launcher_path: canonicalLauncherPath, })); const localOwner = discoverLocalOwnership({ packageName, launcherPath }); if (localOwner) { candidates.push({ manager: localOwner.manager, scope: "local", package_name: packageName, install_root: path.resolve(localOwner.installRoot), - launcher_path: realpathSync(launcherPath), + launcher_path: canonicalLauncherPath, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@npm/satelle/lib/launcher.cjs` around lines 305 - 331, Update packageInstallContext to resolve launcherPath once before building candidates, catch realpathSync failures, and return undefined when resolution fails. Reuse the resolved path for both global and local candidate objects, preserving the existing fail-closed behavior.npm/test/npm-distribution.test.cjs (1)
505-521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test asserts names only, not executability.
The stubbed
runCommandbypassesspawnSync, sonpm.cmdandpnpm.cmdare never launched. The Windows spawn behavior flagged onlauncher.cjslines 162-184 stays uncovered. Add one test that exercises the realcommandLineagainst a.cmdshim, or gate it toprocess.platform === "win32".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@npm/test/npm-distribution.test.cjs` around lines 505 - 521, Extend the Windows global discovery coverage around launcher.discoverGlobalOwnership so it exercises actual command execution rather than only recording command names: invoke the real commandLine path with a .cmd shim and verify the Windows spawn behavior, or condition the test on process.platform === "win32" when using native execution. Preserve the existing assertions for npm.cmd, pnpm.cmd, and bun.exe.npm/test/release-followup.test.cjs (1)
173-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the self-hosted runner assertion to the whole workflow.
This assertion inspects
buildMatrixonly. Thelifecyclejob declares its own runner matrix, and the attestation policy innpm/scripts/release.cjspasses--deny-self-hosted-runners, so a self-hosted runner reintroduced in the lifecycle matrix would pass this test and then fail late inattest. Assert againstworkflowinstead.Proposed change
assert.doesNotMatch( - buildMatrix, + workflow, /runner:.*self-hosted/, "pull-request release builds must not run untrusted code on persistent runners", );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@npm/test/release-followup.test.cjs` around lines 173 - 177, Update the self-hosted runner assertion in the release-followup test to inspect the full workflow object instead of only buildMatrix. Keep the existing doesNotMatch pattern and failure message, ensuring runner declarations in both build and lifecycle matrices are covered.crates/satelle-cli/src/transport.rs (1)
4555-4571: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead digest read in
verified_host_update_artifact_from_metadata.Line 4564,
let _verified_digest = metadata.digest();, computes the digest and discards it.digest()is a pure accessor with no side effect, and the local binding is never compared or used. The underscore prefix suppresses the unused-variable lint rather than fixing the dead code.The name
_verified_digestimplies a verification step happens here. It does not: the actual digest comparison against the expected value happens later, inssh_bootstrap::DownloadedArtifact::fetch_with_metadata. Remove this line so the code does not suggest a verification step that does not exist at this location.Proposed fix
let metadata = metadata.map_err(|error| map_release_artifact_error(host, version, target, error))?; - let _verified_digest = metadata.digest(); Ok(crate::host_update::VerifiedHostArtifact { version: version.to_string(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/satelle-cli/src/transport.rs` around lines 4555 - 4571, Remove the unused `_verified_digest` assignment from `verified_host_update_artifact_from_metadata`; retain the metadata error mapping and `VerifiedHostArtifact` construction unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.facts:
- Around line 308-316: Update the .facts entry describing the public GitHub
release workflow so it only states that the workflow creates or updates a draft
release, attaches release assets and metadata, and validates the release
artifact set. Remove the claim that it publishes the release after validation,
keeping the surrounding publishing facts unchanged.
In `@crates/satelle-cli/src/error-output.rs`:
- Around line 400-404: Separate UnsupportedLocalPlatform and
UnsupportedReleaseTarget from the input-error contract in the error-contract
mapping. Add a dedicated arm using ErrorCategory::InvalidRequest, retryable
false, outcome describing that the platform or release target is outside the
Controller matrix, and default recovery directing the user to select a supported
Controller target; keep their existing exit class unchanged.
In `@npm/satelle/lib/launcher.cjs`:
- Around line 186-199: Update outerNodeModulesRoot to return the innermost
node_modules ancestor that directly contains the relevant package root, rather
than retaining the outermost match. Ensure the install_root passed to native
receipt validation corresponds to the Bun global installation when nested
node_modules trees and parent project roots both exist, including the analogous
logic at the other reported occurrence.
- Around line 485-492: Update executeNativeBinary so it removes every
case-insensitive variant of packageInstallContextEnvironment from the copied
environment before optionally injecting the serialized installContext. Preserve
the existing exact-key behavior and ensure only the intended context variable is
re-added when installContext is provided.
- Around line 162-184: Update commandLine and its callers so Windows .cmd probes
use spawnSync with shell enabled, while non-.cmd commands retain direct
spawning. Keep manager arguments passed as separate, non-interpolated arguments,
and preserve the existing status/error and single-line output handling so
packageManagerCommand-based npm/pnpm ownership detection returns manager-native
guidance.
---
Nitpick comments:
In `@crates/satelle-cli/src/transport.rs`:
- Around line 4555-4571: Remove the unused `_verified_digest` assignment from
`verified_host_update_artifact_from_metadata`; retain the metadata error mapping
and `VerifiedHostArtifact` construction unchanged.
In `@npm/satelle/lib/launcher.cjs`:
- Around line 305-331: Update packageInstallContext to resolve launcherPath once
before building candidates, catch realpathSync failures, and return undefined
when resolution fails. Reuse the resolved path for both global and local
candidate objects, preserving the existing fail-closed behavior.
In `@npm/test/npm-distribution.test.cjs`:
- Around line 505-521: Extend the Windows global discovery coverage around
launcher.discoverGlobalOwnership so it exercises actual command execution rather
than only recording command names: invoke the real commandLine path with a .cmd
shim and verify the Windows spawn behavior, or condition the test on
process.platform === "win32" when using native execution. Preserve the existing
assertions for npm.cmd, pnpm.cmd, and bun.exe.
In `@npm/test/release-followup.test.cjs`:
- Around line 173-177: Update the self-hosted runner assertion in the
release-followup test to inspect the full workflow object instead of only
buildMatrix. Keep the existing doesNotMatch pattern and failure message,
ensuring runner declarations in both build and lifecycle matrices are covered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac194434-7739-4ad7-8dff-3783e5506113
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.facts.github/workflows/release.ymlCargo.tomlcrates/satelle-cli/Cargo.tomlcrates/satelle-cli/src/error-output.rscrates/satelle-cli/src/main.rscrates/satelle-cli/src/self-update.rscrates/satelle-cli/src/ssh-bootstrap.rscrates/satelle-cli/src/transport-tests.rscrates/satelle-cli/src/transport.rscrates/satelle-cli/tests/cli.rscrates/satelle-core/src/lib.rscrates/satelle-transport/src/server/host_error.rsnpm/satelle/lib/launcher.cjsnpm/satelle/platforms.jsonnpm/scripts/release.cjsnpm/test/npm-distribution.test.cjsnpm/test/release-followup.test.cjsnpm/test/release-infrastructure.test.cjs
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c02ab5fecf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/satelle-cli/src/ssh-bootstrap.rs`:
- Around line 5284-5290: Update the transport error conversion around the
VerifiedRelease handling in transport.rs so non-unavailable SelfUpdateError
paths construct SatelleError with the original version and target preserved,
rather than using only into_satelle_error(). Keep unavailable handling intact
while adding coverage for required details in both unavailable and
non-unavailable paths, including consistent CLI/MCP machine-readable fields and
schema behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 45cf1a37-fb9a-4e8b-8f95-6881e34d83e9
📒 Files selected for processing (5)
crates/satelle-cli/src/self-update.rscrates/satelle-cli/src/ssh-bootstrap.rscrates/satelle-cli/src/tailscale.rscrates/satelle-cli/src/transport.rscrates/satelle-cli/tests/mcp.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/satelle-cli/tests/mcp.rs
- crates/satelle-cli/src/transport.rs
- crates/satelle-cli/src/self-update.rs
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai rate limit |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 33 minutes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fbacad7e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai rate limit |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 39 minutes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 586d9d126f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Train
e1daf5de4be23cd839ca8ee42e849030e060aeed75cfbebda60deff3b7ffe5f82cbcbc74c63d3f9f8789e20aec433237b70376e92c868de26f250b01^..75cfbebda60deff3b7ffe5f82cbcbc74c63d3f9f2b31bd1f09eb20c9448179877e5aecc80040c4177d6645135980e8aa689c6f21What this ships
Packet boundary
Packet 24 is already delivered on main by original PR #150 at
f19ff0cc8d46f28364198887d88cc56c864a481e, so this PR does not manufacture a duplicate packet 24 diff.GAP-054 records re-exec as minimum support for packet 25 fact L227. Packet 26 still owns explicit multi-Host selectors, combined consent, bounded concurrency, partial failure, and aggregate output. Prerelease self-updates remain local-only because the packet 25 Host update contract accepts stable releases. The internal review suggestion to make
--no-inputsilently imply--yeswas rejected because that would bypass the Host update consent contract.Verification
@implementedfacts lint --file .factsOut of scope
Summary by CodeRabbit
New Features
--yesconfirmation.Bug Fixes
Documentation
Greptile Summary
This PR adds verified local self-update and stable single-Host update handoff.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant CLI as Current satelle participant Release as Verified release artifacts participant NewCLI as Replaced satelle participant Host as Selected remote Host User->>CLI: satelle self update CLI->>Release: Fetch archive, checksum, and attestation Release-->>CLI: Verified version-matched artifact CLI->>CLI: Lock, replace executable, and record receipt opt Stable single-Host handoff accepted CLI->>NewCLI: Re-execute installed binary NewCLI->>Host: Plan, confirm, and apply Host update Host-->>NewCLI: Updated Host result endReviews (13): Last reviewed commit: "fix: suppress unsupported prerelease Hos..." | Re-trigger Greptile
Context used (4)