integrations: Add install integration test runner#21
Merged
davidabram merged 7 commits intomainfrom Apr 8, 2026
Merged
Conversation
Add standalone Rust crate under integrations/install/ with shared harness and npm/Bun/Cargo channel flows. Wire flake.nix for build, checks, and thin app delegation. Update .gitignore for integrations/**/target/. Co-authored-by: SCE <sce@crocoder.dev>
|
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:
📝 WalkthroughWalkthroughAdds a new integration test crate Changes
Sequence DiagramsequenceDiagram
participant Main as Main (binary)
participant CLI as CLI Parser
participant Runner as ChannelRunner
participant Harness as ChannelHarness
participant Installer as Package Manager (npm / bun / cargo)
participant SCE as SCE Binary
Main->>CLI: parse args -> channels
Main->>Runner: runner.run(&channels, repo_root)
loop per channel
Runner->>Harness: ChannelHarness::new(channel)
Runner->>Installer: channel::run(HarnessRequest, repo_root)
Installer->>Harness: build fixture / stage binaries
Installer->>Harness: run_command_in_dir_with_env(install command)
Harness->>Installer: command result (status, stdout, stderr)
alt install succeeded
Installer->>Harness: resolve_sce_binary
Harness->>SCE: run "sce --version" / "sce doctor"
SCE-->>Harness: outputs
Harness-->>Installer: validate version OK
Installer-->>Runner: Ok
else install failed
Harness-->>Installer: return InstallFailed (with outputs)
Installer-->>Runner: Err
end
end
Runner-->>Main: Result (Ok / Err)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 637-649: The packaged app installChannelIntegrationTestsApp is
missing Bun and Cargo on PATH causing harness.resolve_program() to fail at
runtime; update runtimeInputs to include pkgs.bun and the Cargo binary (e.g.,
pkgs.cargo or pkgs.rustPackages.cargo depending on your pkgs set) so bun and
cargo are available at runtime, and remove scePackage from runtimeInputs since
the script already references it directly via ${scePackage}/bin/sce; ensure
integrationsInstallPackage remains in runtimeInputs so exec
"${integrationsInstallPackage}/bin/install-channel-integration-tests" works.
In `@integrations/install/src/channels/npm.rs`:
- Around line 185-202: The current add_runtime_to_staged_package_manifest
function performs a brittle string replace; instead read and parse package.json
into a serde_json::Value, locate (or create) the "files" array, ensure the
string "runtime" is present (push it only if missing), then serialize the JSON
back to a string and write it to package_json_path; update error messages in
add_runtime_to_staged_package_manifest to reflect JSON parsing/serialization
failures and preserve other fields/formatting by using
serde_json::to_string_pretty (or to_string) when writing.
- Around line 76-91: find_repo_root currently always walks from
std::env::current_dir(), which fails when the wrapper execs this binary from
elsewhere; change find_repo_root to accept an explicit repo root argument (e.g.,
Option<PathBuf> or Option<&Path>) provided by the wrapper and use that if
supplied and contains "flake.nix", otherwise fall back to the existing
upward-walk from current_dir(); validate that the provided path has flake.nix
and return an Err if neither the explicit path nor the fallback search finds
flake.nix. Ensure references to find_repo_root and the presence check for
"flake.nix" are updated accordingly.
- Around line 17-18: The post-install check is incorrectly using
resolve_program("sce") which can fall back to the original PATH via
resolve_executable() and path_with_harness_bins() because
path_with_harness_bins() extends with self.original_path; instead, restrict
resolution to harness-managed bins only by adding/using a resolver that does not
append original_path (e.g., create resolve_program_in_harness_bins or a flag on
resolve_executable) and call that from the post-install validation (where
harness.resolve_program("sce") is used); update
path_with_harness_bins()/resolve_executable() to provide a variant that omits
self.original_path and use that variant for the NPM post-install validation so
only harness bin dirs are searched.
In `@integrations/install/src/cli.rs`:
- Around line 3-35: Add unit tests next to parse_args that pin the CLI contract
for Command/USAGE behavior: write tests that call parse_args with (1) "--help"
and assert it returns Ok(Command::Help(USAGE.to_string())), (2) no args and
assert default returns Ok(Command::Run { channels } ) where
Channel::from_selector("all") is used, (3) "--channel" with no value and assert
Err contains the exact missing-value message currently constructed, (4) an
unknown argument and assert Err matches the Unknown argument message including
USAGE, and (5) an unsupported selector (e.g. "--channel invalid") and assert
Channel::from_selector returns the expected error; use the parse_args function,
Command enum variants, USAGE constant and Channel::from_selector to locate
behavior and assert exact strings.
In `@integrations/install/src/harness.rs`:
- Around line 7-8: Group the two separate std::process imports into a single
grouped import to follow the style guide: replace the separate lines using the
symbols Command and ExitStatus with a single statement that imports both (use
std::process::{Command, ExitStatus};) so both types come from the same grouped
std::process import.
- Around line 237-241: The branch in resolve_executable that returns early for
path-containing program arguments skips validation; change it so that when
program contains path separators you convert to a PathBuf and call the same
existence/executable check used for bare names (e.g., ensure_executable or the
method that validates a Path) and return an Err with the same error format on
failure, otherwise return the PathBuf; reference resolve_executable and
ensure_executable to locate where to apply the check so explicit paths are
validated the same way as looked-up programs.
- Around line 415-426: The current loop in copy_directory_recursive silently
ignores entries where entry_type is neither a dir nor file (e.g., symlinks);
update the logic to explicitly handle symlinks by checking
entry_type.is_symlink() and returning a clear Err (including entry.path() and
destination_path) instead of skipping them so tests fail fast if fixtures
contain symlinks; locate the branch in copy_directory_recursive that inspects
entry_type and add an else if for is_symlink() that constructs and returns a
descriptive error.
In `@integrations/install/src/main.rs`:
- Around line 20-31: Change run() to return anyhow::Result<()> instead of
Result<(), String>, propagate errors from parse_args and ChannelRunner::run
without formatting, and add context where I/O or process failures occur using
anyhow::Context/with_context in parse_args and ChannelRunner methods; then
render/format the error message and set exit codes in main() so service-layer
code returns rich anyhow errors while main() performs final human-facing error
rendering and exit classification.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dce229ea-d41b-490e-959d-a76c7fbeba5b
⛔ Files ignored due to path filters (8)
context/architecture.mdis excluded by!context/**/*.mdcontext/context-map.mdis excluded by!context/**/*.mdcontext/glossary.mdis excluded by!context/**/*.mdcontext/overview.mdis excluded by!context/**/*.mdcontext/patterns.mdis excluded by!context/**/*.mdcontext/plans/optional-nix-install-channel-integration-tests.mdis excluded by!context/**/*.mdcontext/sce/optional-install-channel-integration-test-entrypoint.mdis excluded by!context/**/*.mdintegrations/install/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.gitignoreflake.nixintegrations/install/Cargo.tomlintegrations/install/src/channels/bun.rsintegrations/install/src/channels/cargo.rsintegrations/install/src/channels/mod.rsintegrations/install/src/channels/npm.rsintegrations/install/src/cli.rsintegrations/install/src/harness.rsintegrations/install/src/main.rs
Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
integrations/install/src/cli.rs (1)
9-35: 🛠️ Refactor suggestion | 🟠 MajorAdd close-by tests for this CLI contract.
ArgsandChannelArgnow own the stable--channelsurface, but there are still no local tests pinning the defaultallbehavior, selector parsing, and rendered help/error output.Based on learnings, "Assert on exact output when the CLI contract is supposed to be stable" and "Add unit tests close to the code they exercise".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/cli.rs` around lines 9 - 35, Add unit tests next to the CLI (in the same module or a sibling tests module) that assert the stable CLI contract for Args/ChannelArg: 1) test default when no args are passed yields ChannelArg::All by calling Args::try_parse_from(&["install-channel-integration-tests"]) and converting Args.channel into Vec<Channel> (use the From<ChannelArg> impl); 2) test explicit selectors parse correctly (e.g., "--channel npm", "bun", "cargo") by asserting the produced Vec<Channel>; and 3) test help and error rendering by invoking the clap command generator (use Args::command() or Args::command().render_help()/try_get_matches_from to capture output) and assert exact help text and expected error message for invalid values. Reference Args, ChannelArg, and the From<ChannelArg> for locating the code.integrations/install/src/channels/npm.rs (3)
77-92:⚠️ Potential issue | 🟠 MajorDon't anchor repo-root discovery to the caller's working directory.
The flake app wrapper can exec this binary from outside the checkout, so walking upward only from
current_dir()makes the runner fail before any channel flow begins. Accept an explicit repo root from the wrapper and use the cwd walk only as a fallback. This helper is also reused byintegrations/install/src/channels/bun.rsLines 6-14, so the same failure mode affects both channels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/channels/npm.rs` around lines 77 - 92, The function find_repo_root currently walks from std::env::current_dir(); change its API to accept an optional explicit repo root (e.g., add a parameter like repo_root: Option<&Path>) and use that path first to check for "flake.nix", falling back to walking up from current_dir() only if the explicit path is None or doesn't contain the file; update callers (including integrations/install/src/channels/bun.rs and any invocations of find_repo_root) to pass the wrapper-provided repo root when available, and preserve the existing HarnessError cases (RepoRootMissing/CurrentDir) semantics when both explicit path and cwd fallback fail.
175-192:⚠️ Potential issue | 🟠 MajorPatch
package.jsonstructurally, not by exact whitespace.This replacement only succeeds while the staged manifest still contains the exact
"lib",\n\t\t"README.md"fragment fromnpm/package.jsonLines 10-14. A reformat, a reorder offiles, or an existingruntimeentry turns unrelated manifest edits into harness failures. Parse the JSON and idempotently ensure"runtime"exists in"files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/channels/npm.rs` around lines 175 - 192, The current add_runtime_to_staged_package_manifest function should not use a brittle string replace; instead parse the package.json with serde_json (or equivalent), locate or create the "files" array, ensure the string "runtime" is present (idempotently), then write the updated JSON back to package_json_path; update error branches to return the same HarnessError::FileRead/Write/ManifestInject as appropriate and only return ManifestInject when the files array already contained "runtime" (or skip returning an error and treat it as a no-op). Use the existing function name add_runtime_to_staged_package_manifest and the package_json_path argument to find the code to change.
18-20:⚠️ Potential issue | 🟠 MajorResolve the installed launcher from harness-managed bins only.
resolve_program("sce")searchesChannelHarness::path_with_harness_bins()inintegrations/install/src/harness.rsLines 318-325, and that helper appends the originalPATH. If the global npm install never writessceinto the harness prefix, this can still pick up an unrelated host binary and report a false pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/channels/npm.rs` around lines 18 - 20, The test currently uses harness.resolve_program("sce") which allows falling back to the original PATH; update the test to locate the launcher strictly from the harness-managed bins by using ChannelHarness::path_with_harness_bins() (or a dedicated harness-only resolver) to search only that bin directory instead of the full PATH. Concretely: replace the call to resolve_program("sce") with a lookup that inspects the path returned by path_with_harness_bins() (or add a new method like resolve_program_in_harness_bins) so resolve/lookup of "sce" cannot pick up a host/global binary, then keep the rest of the assertions (assert_sce_version_success, assert_sce_doctor_success) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integrations/install/src/channels/npm.rs`:
- Around line 204-213: The current failure path incorrectly uses
HarnessError::CommandFailed for a completed process; add a new error variant
(e.g., HarnessError::CommandExitedNonZero or
HarnessError::CommandFailedWithStatus) that carries the exit status, stdout, and
stderr (in addition to channel and program), update the HarnessError enum and
its constructors/matches accordingly, and replace the return here to construct
and return that new variant using output.status, output.stdout, and
output.stderr so callers can inspect exit code and both output streams for
diagnostics.
In `@integrations/install/src/harness.rs`:
- Around line 159-160: resolve_program currently forwards to resolve_executable
with the same env hint used for resolve_sce_binary, causing errors for missing
package managers to suggest SCE_INSTALL_CHANNEL_SCE_BIN which cannot fix them;
change resolve_program(&self, program: &str) to call
self.resolve_executable(OsStr::new(program)) without supplying the
SCE_INSTALL_CHANNEL_SCE_BIN env hint and ensure its returned HarnessError
message only mentions PATH lookup; keep the env hint logic and more specific
SCE_INSTALL_CHANNEL_SCE_BIN suggestion inside resolve_sce_binary (and where it
sets env) so only SCE-specific resolution suggests that override.
In `@integrations/install/src/main.rs`:
- Around line 9-12: Reorder and normalize the import block so it follows the
repo convention: standard library imports first (none here), then third-party
crates (e.g., clap::Parser), and finally crate-local imports using crate::...
style; replace local module imports (channels::ChannelRunner, cli::Args,
error::HarnessError) with crate::channels::ChannelRunner, crate::cli::Args,
crate::error::HarnessError and place them after the third-party imports
(clap::Parser) to match the std / third-party / crate::... ordering.
In `@integrations/install/src/platform/unix.rs`:
- Around line 20-31: The function set_executable_permissions currently
overwrites file mode with 0o755; instead read the current mode from
fs::metadata(path).permissions() (use std::os::unix::fs::PermissionsExt to
access mode()), compute new_mode = current_mode | 0o111 to add execute bits
only, then call permissions.set_mode(new_mode) and fs::set_permissions(path,
permissions); update error handling in the same HarnessError branches and ensure
PermissionsExt is imported.
- Around line 7-15: The ensure_executable function currently maps every failure
to HarnessError::UnixOnly; instead preserve specific causes and attach I/O
context: when calling path.metadata(), map the underlying io::Error into a more
descriptive HarnessError (or wrap it) using Context/with_context so callers can
see the original error; if metadata.is_file() is false return a distinct error
(e.g. HarnessError::NotAFile or a variant that indicates "not a file"); if the
execute bits are missing return a specific permission error (e.g.
HarnessError::MissingExecutePermission); reference ensure_executable,
path.metadata(), metadata.is_file(), and metadata.permissions().mode() when
making these changes so the callers get accurate diagnostics rather than
overloaded UnixOnly.
---
Duplicate comments:
In `@integrations/install/src/channels/npm.rs`:
- Around line 77-92: The function find_repo_root currently walks from
std::env::current_dir(); change its API to accept an optional explicit repo root
(e.g., add a parameter like repo_root: Option<&Path>) and use that path first to
check for "flake.nix", falling back to walking up from current_dir() only if the
explicit path is None or doesn't contain the file; update callers (including
integrations/install/src/channels/bun.rs and any invocations of find_repo_root)
to pass the wrapper-provided repo root when available, and preserve the existing
HarnessError cases (RepoRootMissing/CurrentDir) semantics when both explicit
path and cwd fallback fail.
- Around line 175-192: The current add_runtime_to_staged_package_manifest
function should not use a brittle string replace; instead parse the package.json
with serde_json (or equivalent), locate or create the "files" array, ensure the
string "runtime" is present (idempotently), then write the updated JSON back to
package_json_path; update error branches to return the same
HarnessError::FileRead/Write/ManifestInject as appropriate and only return
ManifestInject when the files array already contained "runtime" (or skip
returning an error and treat it as a no-op). Use the existing function name
add_runtime_to_staged_package_manifest and the package_json_path argument to
find the code to change.
- Around line 18-20: The test currently uses harness.resolve_program("sce")
which allows falling back to the original PATH; update the test to locate the
launcher strictly from the harness-managed bins by using
ChannelHarness::path_with_harness_bins() (or a dedicated harness-only resolver)
to search only that bin directory instead of the full PATH. Concretely: replace
the call to resolve_program("sce") with a lookup that inspects the path returned
by path_with_harness_bins() (or add a new method like
resolve_program_in_harness_bins) so resolve/lookup of "sce" cannot pick up a
host/global binary, then keep the rest of the assertions
(assert_sce_version_success, assert_sce_doctor_success) unchanged.
In `@integrations/install/src/cli.rs`:
- Around line 9-35: Add unit tests next to the CLI (in the same module or a
sibling tests module) that assert the stable CLI contract for Args/ChannelArg:
1) test default when no args are passed yields ChannelArg::All by calling
Args::try_parse_from(&["install-channel-integration-tests"]) and converting
Args.channel into Vec<Channel> (use the From<ChannelArg> impl); 2) test explicit
selectors parse correctly (e.g., "--channel npm", "bun", "cargo") by asserting
the produced Vec<Channel>; and 3) test help and error rendering by invoking the
clap command generator (use Args::command() or
Args::command().render_help()/try_get_matches_from to capture output) and assert
exact help text and expected error message for invalid values. Reference Args,
ChannelArg, and the From<ChannelArg> for locating the code.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57376e03-166f-4fc4-a464-08489769be70
⛔ Files ignored due to path filters (3)
context/plans/doctor-human-text-integration-audit.mdis excluded by!context/**/*.mdcontext/plans/drop-macos-intel-release-support.mdis excluded by!context/**/*.mdintegrations/install/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
integrations/install/Cargo.tomlintegrations/install/src/channels/bun.rsintegrations/install/src/channels/cargo.rsintegrations/install/src/channels/mod.rsintegrations/install/src/channels/npm.rsintegrations/install/src/cli.rsintegrations/install/src/error.rsintegrations/install/src/harness.rsintegrations/install/src/main.rsintegrations/install/src/platform/mod.rsintegrations/install/src/platform/unix.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-sce.yml:
- Around line 168-171: The release workflow uses mutable action tags; update the
four `uses:` entries in the `release` job to pinned commit SHAs: replace
`DeterminateSystems/determinate-nix-action@v3.17.3` with
`DeterminateSystems/determinate-nix-action@32cb6a5ae30bb0dfc996fa7baf8bf1ed28442fa4`,
replace `actions/download-artifact@v8` with
`actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c`, and
likewise pin `actions/checkout@v6` and `softprops/action-gh-release@v2` to their
full commit SHAs once you resolve the correct SHA values; update the `uses:`
strings for the corresponding steps so the release job references immutable
commits.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 371f40d8-05e7-49ea-a20b-e348860ca297
📒 Files selected for processing (6)
.github/workflows/publish-crates.yml.github/workflows/publish-npm.yml.github/workflows/release-sce-linux-arm.yml.github/workflows/release-sce-linux.yml.github/workflows/release-sce-macos-arm.yml.github/workflows/release-sce.yml
…ution Add --repo-root argument to allow explicit repository root path instead of relying solely on upward directory walk from current directory. The explicit path is validated by checking for flake.nix presence. Refactor harness binary resolution to support searching only within harness-managed bin directories (npm prefix, bun install, cargo home) via resolve_program_in_harness_bins(). This ensures channel tests resolve sce from the correct installation source. Co-authored-by: SCE <sce@crocoder.dev>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
integrations/install/src/harness.rs (3)
5-6: 🧹 Nitpick | 🔵 TrivialGroup
std::processimports.As per coding guidelines, "Use grouped
stdimports such asuse std::path::{Path, PathBuf};".✨ Suggested grouping
-use std::process::Command; -use std::process::ExitStatus; +use std::process::{Command, ExitStatus};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/harness.rs` around lines 5 - 6, Group the two separate std::process imports into a single grouped import to follow the style guideline: replace the two lines importing Command and ExitStatus with a single grouped import using the std::process::{Command, ExitStatus} syntax so the declarations around Command and ExitStatus in harness.rs are consolidated.
270-273:⚠️ Potential issue | 🟡 MinorPath arguments bypass existence and executability checks.
When
programcontains path separators (e.g.,./sceor/usr/bin/sce), the function returns immediately without verifying the file exists or is executable. A non-existent path will only fail later duringCommand::output(), producing a less specific error.🔧 Proposed fix
fn resolve_executable_in_paths( &self, program: &OsStr, paths: &OsStr, ) -> Result<PathBuf, HarnessError> { let candidate = Path::new(program); if candidate.components().count() > 1 { + ensure_executable(candidate).map_err(|reason| HarnessError::ExecutableNotFound { + channel: self.channel.as_str().to_string(), + path: candidate.to_path_buf(), + reason: format!("{reason:?}"), + })?; return Ok(candidate.to_path_buf()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/harness.rs` around lines 270 - 273, The early return when Path::new(program) contains path separators (variable candidate) bypasses existence/executability checks; instead, before returning candidate.to_path_buf() validate that the path exists and is executable (use fs::metadata/candidate.exists() and check executable permission on Unix via metadata().permissions() or an appropriate cross-platform check), and if not return a clear Err indicating missing or non-executable target so callers fail fast with a specific error rather than later at Command::output().
282-286:⚠️ Potential issue | 🟡 Minor
SCE_INSTALL_CHANNEL_SCE_BINhint is misleading for generic program lookups.
resolve_executable_in_pathsis used by bothresolve_sce_binary()andresolve_program(). The error message always suggestsSCE_INSTALL_CHANNEL_SCE_BIN, but this env var cannot help whennpm,bun, orcargois missing. Consider parameterizing the error or only including the env hint when resolving thescebinary specifically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/harness.rs` around lines 282 - 286, The error returned from resolve_executable_in_paths always includes the SCE_BINARY_ENV hint, which is misleading when called from resolve_program() for generic tools (npm, bun, cargo); update resolve_executable_in_paths (or its callers) to only include the SCE_INSTALL_CHANNEL_SCE_BIN / SCE_BINARY_ENV hint when resolving the sce binary (e.g., from resolve_sce_binary()), or accept a parameter/flag that controls whether to attach the env-hint to the HarnessError::ExecutableResolve payload; adjust the construction of HarnessError::ExecutableResolve so program, channel, and env are set appropriately (omit or set env to None/empty for generic program lookups) to avoid suggesting an irrelevant env var.integrations/install/src/channels/npm.rs (2)
195-204:⚠️ Potential issue | 🟠 MajorPatch
package.jsonstructurally, not by exact whitespace.This string replacement only succeeds while the manifest contains the exact substring
"lib",\n\t\t"README.md". A reformat, reorder offiles, or future change will break unrelated manifest edits. Parse the JSON and addruntimeto thefilesarray instead.🔧 Proposed structural approach
fn add_runtime_to_staged_package_manifest(package_json_path: &Path) -> Result<(), HarnessError> { - let package_json = - fs::read_to_string(package_json_path).map_err(|e| HarnessError::FileRead { - path: package_json_path.to_path_buf(), - error: e.to_string(), - })?; - let updated_package_json = package_json.replace( - "\"lib\",\n\t\t\"README.md\"", - "\"lib\",\n\t\t\"runtime\",\n\t\t\"README.md\"", - ); - - if updated_package_json == package_json { - return Err(HarnessError::ManifestInject { - path: package_json_path.to_path_buf(), - }); - } - - fs::write(package_json_path, updated_package_json).map_err(|e| HarnessError::FileWrite { - path: package_json_path.to_path_buf(), - error: e.to_string(), - }) + let content = fs::read_to_string(package_json_path).map_err(|e| HarnessError::FileRead { + path: package_json_path.to_path_buf(), + error: e.to_string(), + })?; + + let mut manifest: serde_json::Value = serde_json::from_str(&content) + .map_err(|e| HarnessError::ManifestParse { + path: package_json_path.to_path_buf(), + error: e.to_string(), + })?; + + let files = manifest.get_mut("files") + .and_then(|v| v.as_array_mut()) + .ok_or_else(|| HarnessError::ManifestInject { + path: package_json_path.to_path_buf(), + })?; + + if !files.iter().any(|v| v.as_str() == Some("runtime")) { + files.push(serde_json::json!("runtime")); + } + + let updated = serde_json::to_string_pretty(&manifest) + .map_err(|e| HarnessError::ManifestSerialize { + path: package_json_path.to_path_buf(), + error: e.to_string(), + })?; + + fs::write(package_json_path, updated).map_err(|e| HarnessError::FileWrite { + path: package_json_path.to_path_buf(), + error: e.to_string(), + }) }This requires adding
serde_jsontoCargo.tomland new error variants toHarnessError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/channels/npm.rs` around lines 195 - 204, The current string-replace on package_json is brittle; instead parse the manifest with serde_json, locate or create the "files" array, insert the "runtime" string if not present, and serialize back to a pretty/compact JSON string to compare/write; update the code around updated_package_json/package_json/package_json_path to use serde_json::Value and return HarnessError::ManifestInject (or add a new HarnessError::ManifestParse/Serialize variant) on parse/serialize failures so errors are informative. Ensure you add serde_json to Cargo.toml and update HarnessError enum to include parse/serialize errors referenced by the new logic.
218-228:⚠️ Potential issue | 🟠 MajorUse a dedicated error variant for
sce doctorfailures.
HarnessError::CommandFailedis designed for spawn/exec failures, not for processes that ran but exited non-zero. This loses the exit status and conflates spawn errors with validation failures. As per coding guidelines, "Separate machine classification from rendered messages in Rust when the CLI contract cares about exit codes."🔧 Proposed approach
Add a variant like
HarnessError::CommandNonZeroExit { channel, program, status, stdout, stderr }and use it here to preserve diagnostic information.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/channels/npm.rs` around lines 218 - 228, The code is using HarnessError::CommandFailed for a process that ran but returned a non-zero exit status; add a new error variant HarnessError::CommandNonZeroExit { channel: String, program: String, status: ExitStatus (or i32), stdout: Vec<u8>, stderr: Vec<u8> } in the HarnessError enum and replace the current return in the npm branch with that variant (use the same channel "npm" and sce_binary.display().to_string(), include output.status, output.stdout, and output.stderr) so the non-zero exit is preserved distinctly from spawn/exec failures.integrations/install/src/cli.rs (1)
23-41: 🧹 Nitpick | 🔵 TrivialConsider adding unit tests for the CLI parser.
The CLI defines defaults and value constraints that would benefit from close-by tests. As per coding guidelines, "Add unit tests close to the code they exercise" and "Assert on exact output when the CLI contract is supposed to be stable."
📋 Suggested test coverage
#[cfg(test)] mod tests { use super::*; #[test] fn channel_arg_all_expands_to_all_channels() { let channels: Vec<Channel> = ChannelArg::All.into(); assert_eq!(channels, vec![Channel::Npm, Channel::Bun, Channel::Cargo]); } #[test] fn channel_arg_npm_expands_to_single() { let channels: Vec<Channel> = ChannelArg::Npm.into(); assert_eq!(channels, vec![Channel::Npm]); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/cli.rs` around lines 23 - 41, Add unit tests next to the ChannelArg definition to verify the From<ChannelArg> for Vec<Channel> conversion produces exact expected outputs; create a #[cfg(test)] mod tests with functions like channel_arg_all_expands_to_all_channels and channel_arg_npm_expands_to_single that call let channels: Vec<Channel> = ChannelArg::All.into() and ChannelArg::Npm.into() respectively and assert_eq!(channels, vec![Channel::Npm, Channel::Bun, Channel::Cargo]) and assert_eq!(channels, vec![Channel::Npm]) to lock the CLI contract.integrations/install/src/main.rs (1)
9-12: 🧹 Nitpick | 🔵 TrivialNormalize imports to
crate::...style and correct ordering.As per coding guidelines, "Group Rust imports in this order: standard library, third-party crates, then
crate::...imports."♻️ Proposed cleanup
use std::process::ExitCode; -use channels::ChannelRunner; use clap::Parser; -use cli::Args; -use error::HarnessError; + +use crate::channels::ChannelRunner; +use crate::cli::Args; +use crate::error::HarnessError;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/main.rs` around lines 9 - 12, Reorder and normalize the imports in main.rs so they follow: standard library first, third-party crates second, and crate-local imports using crate::... last; replace cli::Args and error::HarnessError with crate::cli::Args and crate::error::HarnessError (and channels::ChannelRunner with crate::channels::ChannelRunner if it is a local module), keep clap::Parser as third-party, and ensure the use statements reflect that ordering and crate:: qualification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integrations/install/Cargo.toml`:
- Around line 11-15: Add the anyhow crate to Cargo.toml dependencies and update
service-layer functions to return anyhow::Result (or use anyhow::Context) for
richer contextual errors while keeping existing structured error enums created
with thiserror (e.g., HarnessError); specifically, add anyhow = "1" to the
[dependencies] section and replace or wrap function return types in modules that
perform I/O or process operations with anyhow::Result, using
Context/with_context to attach messages before mapping into HarnessError
variants where needed.
In `@integrations/install/src/channels/bun.rs`:
- Around line 39-45: The call to run_command_in_dir_with_env is passing
package_tarball.to_string_lossy().as_ref(), which can drop non-UTF-8 path data;
change to pass the OsStr/Path directly (e.g., package_tarball.as_os_str() or
&package_tarball) so the S: AsRef<OsStr> param gets the original path; update
the same pattern in the bun invocation (symbol: run_command_in_dir_with_env,
variable: package_tarball, command array containing "add", "--global") and
mirror this fix in npm.rs and cargo.rs where paths are converted lossily.
In `@integrations/install/src/channels/cargo.rs`:
- Around line 20-21: Replace the call to harness.resolve_program("sce") with
harness.resolve_program_in_harness_bins("sce") so the post-install validation
resolves the `sce` binary from the harness-managed bins instead of the host
PATH; keep the subsequent call to
harness.assert_sce_version_success(&sce_binary) unchanged so version validation
runs against the harness-resolved binary.
---
Duplicate comments:
In `@integrations/install/src/channels/npm.rs`:
- Around line 195-204: The current string-replace on package_json is brittle;
instead parse the manifest with serde_json, locate or create the "files" array,
insert the "runtime" string if not present, and serialize back to a
pretty/compact JSON string to compare/write; update the code around
updated_package_json/package_json/package_json_path to use serde_json::Value and
return HarnessError::ManifestInject (or add a new
HarnessError::ManifestParse/Serialize variant) on parse/serialize failures so
errors are informative. Ensure you add serde_json to Cargo.toml and update
HarnessError enum to include parse/serialize errors referenced by the new logic.
- Around line 218-228: The code is using HarnessError::CommandFailed for a
process that ran but returned a non-zero exit status; add a new error variant
HarnessError::CommandNonZeroExit { channel: String, program: String, status:
ExitStatus (or i32), stdout: Vec<u8>, stderr: Vec<u8> } in the HarnessError enum
and replace the current return in the npm branch with that variant (use the same
channel "npm" and sce_binary.display().to_string(), include output.status,
output.stdout, and output.stderr) so the non-zero exit is preserved distinctly
from spawn/exec failures.
In `@integrations/install/src/cli.rs`:
- Around line 23-41: Add unit tests next to the ChannelArg definition to verify
the From<ChannelArg> for Vec<Channel> conversion produces exact expected
outputs; create a #[cfg(test)] mod tests with functions like
channel_arg_all_expands_to_all_channels and channel_arg_npm_expands_to_single
that call let channels: Vec<Channel> = ChannelArg::All.into() and
ChannelArg::Npm.into() respectively and assert_eq!(channels, vec![Channel::Npm,
Channel::Bun, Channel::Cargo]) and assert_eq!(channels, vec![Channel::Npm]) to
lock the CLI contract.
In `@integrations/install/src/harness.rs`:
- Around line 5-6: Group the two separate std::process imports into a single
grouped import to follow the style guideline: replace the two lines importing
Command and ExitStatus with a single grouped import using the
std::process::{Command, ExitStatus} syntax so the declarations around Command
and ExitStatus in harness.rs are consolidated.
- Around line 270-273: The early return when Path::new(program) contains path
separators (variable candidate) bypasses existence/executability checks;
instead, before returning candidate.to_path_buf() validate that the path exists
and is executable (use fs::metadata/candidate.exists() and check executable
permission on Unix via metadata().permissions() or an appropriate cross-platform
check), and if not return a clear Err indicating missing or non-executable
target so callers fail fast with a specific error rather than later at
Command::output().
- Around line 282-286: The error returned from resolve_executable_in_paths
always includes the SCE_BINARY_ENV hint, which is misleading when called from
resolve_program() for generic tools (npm, bun, cargo); update
resolve_executable_in_paths (or its callers) to only include the
SCE_INSTALL_CHANNEL_SCE_BIN / SCE_BINARY_ENV hint when resolving the sce binary
(e.g., from resolve_sce_binary()), or accept a parameter/flag that controls
whether to attach the env-hint to the HarnessError::ExecutableResolve payload;
adjust the construction of HarnessError::ExecutableResolve so program, channel,
and env are set appropriately (omit or set env to None/empty for generic program
lookups) to avoid suggesting an irrelevant env var.
In `@integrations/install/src/main.rs`:
- Around line 9-12: Reorder and normalize the imports in main.rs so they follow:
standard library first, third-party crates second, and crate-local imports using
crate::... last; replace cli::Args and error::HarnessError with crate::cli::Args
and crate::error::HarnessError (and channels::ChannelRunner with
crate::channels::ChannelRunner if it is a local module), keep clap::Parser as
third-party, and ensure the use statements reflect that ordering and crate::
qualification.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbedc7b6-2778-49cd-8114-7bd78217151c
⛔ Files ignored due to path filters (1)
integrations/install/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
flake.nixintegrations/install/Cargo.tomlintegrations/install/src/channels/bun.rsintegrations/install/src/channels/cargo.rsintegrations/install/src/channels/mod.rsintegrations/install/src/channels/npm.rsintegrations/install/src/cli.rsintegrations/install/src/harness.rsintegrations/install/src/main.rs
Co-authored-by: SCE <sce@crocoder.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integrations/install/src/channels/bun.rs`:
- Around line 21-23: The post-install validation currently calls
harness.resolve_program("sce") which may resolve the system PATH binary; change
this to harness.resolve_program_in_harness_bins("sce") so the tests validate the
harness-installed binary. Update the code that assigns sce_binary (used by
assert_sce_version_success and assert_sce_doctor_success) to call
resolve_program_in_harness_bins("sce") instead of resolve_program("sce") and
keep the subsequent calls to harness.assert_sce_version_success(&sce_binary)?
and harness.assert_sce_doctor_success(&sce_binary)? unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0865258-f085-4148-8dfb-4dd4106396c3
📒 Files selected for processing (6)
integrations/install/src/channels/bun.rsintegrations/install/src/channels/cargo.rsintegrations/install/src/channels/npm.rsintegrations/install/src/error.rsintegrations/install/src/harness.rsintegrations/install/src/platform/unix.rs
…ness bins only Co-authored-by: SCE <sce@crocoder.dev>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
integrations/install/src/channels/npm.rs (1)
193-214:⚠️ Potential issue | 🟠 MajorUpdate
package.jsonstructurally instead of matching exact whitespace.This only works while the
filesarray keeps the exact"lib",\n\t\t"README.md"layout. A reformat, reordering, or an already-present"runtime"entry turns unrelated manifest changes into a harness failure for both npm and bun. Parse the JSON, ensure"runtime"is present infiles, and write it back idempotently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/channels/npm.rs` around lines 193 - 214, The current add_runtime_to_staged_package_manifest function relies on exact string replacement and fails on formatting or if "runtime" is already present; change it to parse package_json_path into a serde_json::Value, locate or create the "files" array, ensure the string "runtime" is present (idempotently) and only return Err(HarnessError::ManifestInject) if you actually cannot insert; serialize the modified JSON back (e.g., to_string_pretty or to_string) and fs::write it, mapping IO/ser errors to the same HarnessError::FileRead/FileWrite variants; keep references to package_json_path, HarnessError::ManifestInject and the function name add_runtime_to_staged_package_manifest to locate the change.integrations/install/src/channels/cargo.rs (1)
20-22:⚠️ Potential issue | 🟠 MajorResolve the installed
scefrom harness bins only.
resolve_program("sce")still falls back toself.original_path, so a preinstalled hostscecan hide a failedcargo install. Match the npm/bun flows and resolve from harness-managed bins here.🔧 Minimal fix
- let sce_binary = harness.resolve_program("sce")?; + let sce_binary = harness.resolve_program_in_harness_bins("sce")?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/install/src/channels/cargo.rs` around lines 20 - 22, The code is accidentally allowing a host-installed `sce` because harness.resolve_program("sce") falls back to self.original_path; replace that call with a harness-only resolution (e.g., a harness method that resolves only harness-managed bins such as resolve_program_in_harness_bins or resolve_harness_bin("sce")) so the installed binary comes exclusively from the harness-managed install; update the call site that assigns sce_binary (the variable used by assert_sce_version_success and assert_sce_doctor_success) to use this harness-only resolver, or if such a helper doesn't exist, add one on the harness that looks up only harness bins and use it here.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integrations/install/src/error.rs`:
- Around line 38-43: Update the Display annotations for the error variants
SceVersionFailed, NpmInstallFailed, NpmPackFailed, BunInstallFailed, and
CargoInstallFailed to include their captured stdout/stderr fields (or
stderr/stdout Option<String> as appropriate) in the #[error("...")] string,
following the pattern used by CommandExitedNonZero so the process output is
visible when Display is printed; locate those enum variants in error.rs and
append the corresponding {stdout} and/or {stderr} interpolation placeholders to
their format strings while keeping existing context (e.g., channel, path, cmd).
In `@integrations/install/src/harness.rs`:
- Around line 153-166: The test currently only checks exit code in
assert_sce_doctor_success; update that function to also parse output.stdout as
JSON (e.g., via serde_json::from_str::<serde_json::Value>(&output.stdout)) after
run_command returns, and return an Err (with parse error details and the
stdout/stderr) if JSON parsing fails or the parsed value is not the expected
shape, so a launcher that prints human text but exits 0 will fail; reference
assert_sce_doctor_success, run_command, and output.stdout when adding the JSON
parse and mapping the serde_json error into the HarnessError path.
---
Duplicate comments:
In `@integrations/install/src/channels/cargo.rs`:
- Around line 20-22: The code is accidentally allowing a host-installed `sce`
because harness.resolve_program("sce") falls back to self.original_path; replace
that call with a harness-only resolution (e.g., a harness method that resolves
only harness-managed bins such as resolve_program_in_harness_bins or
resolve_harness_bin("sce")) so the installed binary comes exclusively from the
harness-managed install; update the call site that assigns sce_binary (the
variable used by assert_sce_version_success and assert_sce_doctor_success) to
use this harness-only resolver, or if such a helper doesn't exist, add one on
the harness that looks up only harness bins and use it here.
In `@integrations/install/src/channels/npm.rs`:
- Around line 193-214: The current add_runtime_to_staged_package_manifest
function relies on exact string replacement and fails on formatting or if
"runtime" is already present; change it to parse package_json_path into a
serde_json::Value, locate or create the "files" array, ensure the string
"runtime" is present (idempotently) and only return
Err(HarnessError::ManifestInject) if you actually cannot insert; serialize the
modified JSON back (e.g., to_string_pretty or to_string) and fs::write it,
mapping IO/ser errors to the same HarnessError::FileRead/FileWrite variants;
keep references to package_json_path, HarnessError::ManifestInject and the
function name add_runtime_to_staged_package_manifest to locate the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c4a04d9-62b5-4d2f-8715-245d7940564e
📒 Files selected for processing (6)
integrations/install/src/channels/bun.rsintegrations/install/src/channels/cargo.rsintegrations/install/src/channels/npm.rsintegrations/install/src/error.rsintegrations/install/src/harness.rsintegrations/install/src/platform/unix.rs
Add diagnostic output to sce version, npm install, npm pack, bun install, and cargo install failure errors to aid debugging. Co-authored-by: SCE <sce@crocoder.dev>
ivke995
approved these changes
Apr 8, 2026
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.
Add standalone Rust crate under integrations/install/ with shared harness and npm/Bun/Cargo channel flows. Wire flake.nix for build, checks, and thin app delegation. Update .gitignore for integrations/**/target/.
Summary by CodeRabbit
Tests
Chores