Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions .github/workflows/rust-windows.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
name: rust-windows

on:
push:
branches: [ master ]
pull_request:
branches: [ '**' ]
types: [opened, synchronize, reopened, ready_for_review]
merge_group:

Comment on lines +3 to +10

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR build can retain write token

This pull_request workflow executes checkout-controlled Cargo, PowerShell, and Bash code without an explicit token scope, so permissive repository defaults can expose write access during same-repository PRs — should we set permissions: contents: read and grant no other permissions?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`.github/workflows/rust-windows.yml` around lines 3-10, the `rust-windows` workflow runs
checkout-controlled build and test code without explicitly restricting `GITHUB_TOKEN`
permissions. Add a workflow-level least-privilege permissions policy granting only
`contents: read` and no other permissions, ensuring repository defaults cannot provide
write access during pull request runs.

env:
CARGO_TERM_COLOR: always
# aws-lc-sys (rustls' default provider) assembles with NASM on x86-64; the
# crate ships prebuilt objects behind this switch, so the runner needs no
# extra install. ARM64 hosts need clang-cl instead.
AWS_LC_SYS_PREBUILT_NASM: 1

jobs:
windows:
strategy:
fail-fast: false
matrix:
include:
- { arch: x86-64, os: windows-latest }
- { arch: arm64, os: windows-11-arm }
runs-on: ${{ matrix.os }}
name: Windows ${{ matrix.arch }}
if: github.event_name != 'pull_request' || github.event.pull_request.draft == false
steps:
- name: Checkout
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
Comment on lines +30 to +31

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Checkout leaves token usable by tests

actions/checkout@v7.0.1 leaves persist-credentials at its documented true default, so checkout-controlled builds/tests and tests/windows/run.ps1 can invoke Git with the persisted token — should we set persist-credentials: false since this job needs no authenticated Git operations afterward?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`.github/workflows/rust-windows.yml` around lines 30-31, update the `actions/checkout`
step to set `persist-credentials` to `false`. This Windows job does not need
authenticated Git operations after checkout, and disabling credential persistence
prevents the subsequent Cargo tests and `tests/windows/run.ps1` from reusing the GitHub
token.

- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@6c977a6ca4077a0ceb28ffbe03f59d46e9ac8772 # stable
with:
toolchain: stable
# aws-lc-sys' ARM assembly is GNU-syntax, which MSVC cannot assemble: the
# build then fails late in lib.exe, archiving objects that were never
# written. clang-cl assembles it and stays ABI-compatible with the MSVC
# toolchain the Rust side links with. The runner image ships it; the step
# reports what it found, and warns loudly if it ever has to install one.
- name: Use clang-cl for C dependencies
if: matrix.arch == 'arm64'
shell: pwsh
run: |
$clang = Get-Command clang-cl -ErrorAction SilentlyContinue
if ($clang) {
Write-Host "clang-cl: $($clang.Source)"
} else {
$found = Get-ChildItem "$env:ProgramFiles\Microsoft Visual Studio", "$env:ProgramFiles\LLVM" `
-Recurse -Filter clang-cl.exe -ErrorAction SilentlyContinue |
Select-Object -First 1 -ExpandProperty DirectoryName
if ($found) {
Write-Host "::warning::clang-cl was not on PATH; using $found"
Add-Content $env:GITHUB_PATH $found
} else {
Write-Host "::warning::the runner image no longer ships clang-cl; installing LLVM"
choco install llvm -y --no-progress
Add-Content $env:GITHUB_PATH "$env:ProgramFiles\LLVM\bin"
Comment on lines +56 to +58

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mutable LLVM package alters CI toolchain

The ARM fallback runs choco install llvm -y --no-progress without pinning the feed, version, or installer digest, so a feed update can change the compiler placed on PATH and used for builds/tests — could we use a pinned, integrity-verified official LLVM artifact or pin all three values?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`.github/workflows/rust-windows.yml` around lines 56-58, update the ARM64 `clang-cl`
fallback installation logic so it cannot resolve an arbitrary LLVM package from
Chocolatey’s default feeds. Use a pinned official LLVM artifact or specify the exact
Chocolatey package version and trusted source, then verify the installer’s expected
SHA-256 digest before installing and adding it to `PATH`. Preserve the existing fallback
behavior and warning if possible, but fail the job when integrity verification fails.

}
}
# cc-rs splits these on whitespace, so the bare name has to resolve on PATH.
Add-Content $env:GITHUB_ENV "CC=clang-cl"
Add-Content $env:GITHUB_ENV "CXX=clang-cl"

# josh-ssh-shell is unix-only (unix sockets, fifos, raw fds), so the
# workspace does not build here.
- name: Build
run: cargo build --locked -p josh-proxy -p josh-cli
# josh compose needs podman and does not run on Windows, so the .t suites
# are out of reach; these are the crates whose unit tests run here.
- name: Unit tests
run: cargo test --locked -p josh-core -p josh-filter -p josh-gix-ext -p josh-git-serde -p josh-search -p josh-memodb
Comment on lines +71 to +72

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Windows CI skips CLI and proxy unit tests

The Windows Unit tests step selects only six packages, so it skips josh-cli and josh-proxy unit tests even though their #[cfg(test)] modules run on Windows — should we add -p josh-cli -p josh-proxy, or describe this as a partial test set?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`.github/workflows/rust-windows.yml` around lines 69-72, update the Windows `Unit tests`
step and its coverage comment. Add `-p josh-cli -p josh-proxy` to the `cargo test
--locked` package list so the unit tests for the binaries built above are executed, and
ensure the comment accurately reflects that these supported packages are covered.

# Drives the built binaries, since the .t suites cannot run here: the CLI
# against a local repository, and josh-proxy against a git server hosted by
# HttpListener, so the job needs nothing that Windows does not ship.
- name: Functional tests
run: pwsh tests/windows/run.ps1 target/debug -PathForms
Comment on lines +73 to +77

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Port collisions invalidate Windows functional tests

The harness hard-codes 8177 and 42190 without reserving them or validating process ownership, so HttpListener.Start() in serve-git.ps1 or josh-proxy can fail while run.ps1 and proxy.sh accept unrelated TCP/HTTP responses as readiness and route traffic to the wrong service. Should we choose free ports, pass them through, and require the expected child plus an identifying response before proceeding?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`.github/workflows/rust-windows.yml` around lines 73-77, fix the `Functional tests`
invocation and its `tests/windows` harness so hard-coded Git server port 8177 and proxy
port 42190 cannot collide with unrelated listeners. Refactor `run.ps1`, `serve-git.ps1`,
and `proxy.sh` as needed to select and pass free ports, verify the expected child
processes remain alive, and require an identifying Git/proxy response before marking
either service ready; fail the test if binding or startup fails.

21 changes: 15 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ serde_json = "1.0.151"
serde_yaml = "0.9.34"
toml = "1.1.4"
tracing-subscriber = { version = "0.3.23", features = ["env-filter"] }
dunce = "1.0.5"
socket2 = "0.6.3"
tempfile = "3.27.0"
hex = "0.4.3"
secret-vault-value = "^1"
Expand Down
1 change: 1 addition & 0 deletions cq/josh-cq/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ anyhow.workspace = true
serde.workspace = true
serde_json.workspace = true
git2.workspace = true
gix-hash.workspace = true
axum.workspace = true
tracing.workspace = true

Expand Down
2 changes: 1 addition & 1 deletion cq/josh-cq/src/cq.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub fn handle_track(
url,
None,
"HEAD",
fetched_commit,
josh_core::objects::gix_oid(fetched_commit),
josh_core::objects::CommitData::read(transaction.odb(), head.commit)?.tree_id()?,
link_mode,
)?
Expand Down
7 changes: 4 additions & 3 deletions cq/josh-cq/src/remote.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
use anyhow::Context;
use anyhow::anyhow;
use std::str::FromStr;

use std::collections::BTreeMap;
use std::process::Command;

/// List refs from a remote repository using git ls-remote
///
/// Returns a map of ref names to their OIDs
pub fn list_refs(url: &str) -> anyhow::Result<BTreeMap<String, git2::Oid>> {
pub fn list_refs(url: &str) -> anyhow::Result<BTreeMap<String, gix_hash::ObjectId>> {
let output = Command::new("git")
.args(["ls-remote", url])
.output()
Expand All @@ -19,12 +20,12 @@ pub fn list_refs(url: &str) -> anyhow::Result<BTreeMap<String, git2::Oid>> {
}

let stdout = String::from_utf8(output.stdout)?;
let refs: BTreeMap<String, git2::Oid> = stdout
let refs: BTreeMap<String, gix_hash::ObjectId> = stdout
.lines()
.filter_map(|line| {
let parts: Vec<&str> = line.split('\t').collect();
if parts.len() == 2 {
let oid = git2::Oid::from_str(parts[0]).ok()?;
let oid = gix_hash::ObjectId::from_str(parts[0]).ok()?;
Some((parts[1].to_string(), oid))
} else {
None
Expand Down
16 changes: 8 additions & 8 deletions devtools/josh-test-support/src/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@ pub fn random_string(rng: &mut StdRng, len: usize) -> String {
pub fn build_index(
repo: &git2::Repository,
sig: &git2::Signature,
heads: &[git2::Oid],
) -> Result<git2::Oid> {
heads: &[gix::ObjectId],
) -> Result<gix::ObjectId> {
let empty_tree = repo.find_tree(repo.treebuilder(None)?.write()?)?;
let parents = heads
.iter()
.map(|oid| repo.find_commit(*oid))
.map(|oid| repo.find_commit(git2_oid(*oid)))
.collect::<Result<Vec<_>, _>>()?;
let parent_refs = parents.iter().collect::<Vec<_>>();
let index = repo.commit(
Expand All @@ -49,7 +49,7 @@ pub fn build_index(
&empty_tree,
&parent_refs,
)?;
Ok(index)
Ok(gix_oid(index))
}

/// Rebuild, with plain git2 tree walking (no josh code), the tree a pattern filter must produce:
Expand All @@ -60,10 +60,10 @@ pub fn build_index(
/// `require_literal_leading_dot`; a glob-based predicate is exact regardless).
pub fn expected_tree(
repo: &git2::Repository,
head: git2::Oid,
head: gix::ObjectId,
keep: &dyn Fn(&str) -> bool,
) -> Result<(git2::Oid, usize)> {
let tree = repo.find_commit(head)?.tree()?;
) -> Result<(gix::ObjectId, usize)> {
let tree = repo.find_commit(git2_oid(head))?.tree()?;
let mut kept: Vec<(String, git2::Oid, i32)> = vec![];
tree.walk(git2::TreeWalkMode::PreOrder, |root, entry| {
if entry.kind() == Some(git2::ObjectType::Blob) {
Expand All @@ -85,5 +85,5 @@ pub fn expected_tree(
};
builder.upsert(path.as_str(), mode, gix_oid(*oid))?;
}
Ok((git2_oid(builder.write()?.detach()), kept.len()))
Ok((builder.write()?.detach(), kept.len()))
}
18 changes: 10 additions & 8 deletions devtools/josh-test-support/src/provision_repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ pub struct ProvisionedRepo {
pub repo: git2::Repository,
/// The head oid the callback produced (always equal to the `expected` that
/// was passed to [`provision_repo`]).
pub head: git2::Oid,
pub head: gix::ObjectId,
}

impl ProvisionedRepo {
Expand All @@ -46,11 +46,11 @@ impl ProvisionedRepo {
/// new `expected`.
pub fn provision_repo<C>(
testcase: &str,
expected: &git2::Oid,
expected: &gix::ObjectId,
callback: C,
) -> Result<ProvisionedRepo>
where
C: FnMut(&git2::Repository) -> Result<git2::Oid>,
C: FnMut(&git2::Repository) -> Result<gix::ObjectId>,
{
let expected = *expected;
let cache_root = cache_root_for(testcase)?;
Expand All @@ -71,17 +71,19 @@ fn cache_root_for(testcase: &str) -> Result<PathBuf> {
}

/// A cached repo is reusable if it opens as a bare repo and contains `expected`.
fn cache_hit(cache_root: &Path, expected: git2::Oid) -> bool {
fn cache_hit(cache_root: &Path, expected: gix::ObjectId) -> bool {
let Ok(repo) = git2::Repository::open_bare(cache_root) else {
return false;
};
repo.odb().map(|odb| odb.exists(expected)).unwrap_or(false)
repo.odb()
.map(|odb| odb.exists(crate::bench::git2_oid(expected)))
.unwrap_or(false)
}

/// Build `testcase` from scratch into `cache_root`, erasing any prior cache.
fn rebuild<C>(cache_root: &Path, expected: git2::Oid, callback: &mut C) -> Result<()>
fn rebuild<C>(cache_root: &Path, expected: gix::ObjectId, callback: &mut C) -> Result<()>
where
C: FnMut(&git2::Repository) -> Result<git2::Oid>,
C: FnMut(&git2::Repository) -> Result<gix::ObjectId>,
{
if cache_root.exists() {
std::fs::remove_dir_all(cache_root)
Expand Down Expand Up @@ -130,7 +132,7 @@ where
}

/// Copy the canonical cached repo into a fresh tempdir and open it.
fn copy_to_tempdir(cache_root: &Path, expected: git2::Oid) -> Result<ProvisionedRepo> {
fn copy_to_tempdir(cache_root: &Path, expected: gix::ObjectId) -> Result<ProvisionedRepo> {
let tmp = tempfile::tempdir().context("creating tempdir for repo copy")?;
copy_dir_recursive(cache_root, tmp.path())
.with_context(|| format!("copying {} to tempdir", cache_root.display()))?;
Expand Down
1 change: 1 addition & 0 deletions docs/src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,6 @@
# Contributing
- [Testing](./contributing/testing.md)
- [Development tools](./contributing/dev-tools.md)
- [Windows](./contributing/windows.md)
- [josh run](./contributing/josh-run.md)
- [Tracing]()
Loading
Loading