Skip to content
Merged
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
50 changes: 44 additions & 6 deletions .githooks/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,41 @@ if command -v rustup >/dev/null 2>&1; then
fi

# 3) Format (auto-fix if opted in)
#
# Auto-stage protocol: when ECHO_AUTO_FMT is on and rustfmt/prettier rewrite
# files, the hook used to abort and force a manual restage-and-recommit cycle
# (paying cargo + lint costs twice). Instead, if the working tree was *clean*
# relative to the index before the formatter ran, the formatted output is
# safe to auto-add: every modified line came from the staged content, so we
# stage the result and continue. Partial staging (any unstaged changes
# present before the formatter) still aborts, because we can't tell whether
# the formatter touched staged hunks, unstaged hunks, or both.
_auto_fmt="${ECHO_AUTO_FMT:-1}"

# Snapshot whether the working tree has unstaged changes BEFORE any
# auto-formatter runs. `git diff --quiet` exits 0 if clean, 1 if dirty.
if git diff --quiet; then
HAD_UNSTAGED_BEFORE_FMT=0
else
HAD_UNSTAGED_BEFORE_FMT=1
fi

case "${_auto_fmt}" in
1|true|TRUE|yes|YES|on|ON)
echo "pre-commit: ECHO_AUTO_FMT=${_auto_fmt} → checking format"
if ! cargo fmt --all -- --check; then
echo "pre-commit: running cargo fmt to apply changes" >&2
cargo fmt --all || { echo "pre-commit: cargo fmt failed" >&2; exit 1; }
echo "pre-commit: rustfmt updated files. Aborting commit to preserve index integrity (partial staging safe)." >&2
echo "Hint: review changes, restage (e.g., 'git add -p' or 'git add -A'), then commit again." >&2
exit 1
if [[ "$HAD_UNSTAGED_BEFORE_FMT" -eq 0 ]]; then
echo "pre-commit: rustfmt updated files; auto-staging (working tree was clean)." >&2
git add -u || { echo "pre-commit: failed to re-stage rustfmt changes" >&2; exit 1; }
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Limit rustfmt restaging to staged files

When cargo fmt --all rewrites any tracked Rust file outside the user's staged set while the worktree is otherwise clean, this git add -u stages every tracked formatter change, not just the files that were part of the commit. In that scenario the hook silently expands the commit with unrelated formatting edits; capture the originally staged paths and restage only those (or keep aborting when rustfmt touches other files).

Useful? React with 👍 / 👎.

# Refresh STAGED so later steps see the post-fmt index.
STAGED=$(git diff --cached --name-only)
else
echo "pre-commit: rustfmt updated files but you had unstaged changes; aborting to preserve partial-staging integrity." >&2
echo "Hint: review changes, restage (e.g., 'git add -p' or 'git add -A'), then commit again." >&2
exit 1
fi
fi
;;
0|false|FALSE|no|NO|off|OFF)
Expand Down Expand Up @@ -139,14 +164,27 @@ if [[ ${#MD_FILES[@]} -gt 0 ]] && command -v npx >/dev/null 2>&1; then
: # Already formatted
else
echo "pre-commit: running prettier on markdown files" >&2
# Re-snapshot working-tree dirtiness for the markdown path specifically.
# The rustfmt step above may have auto-staged its own changes, so the
# earlier HAD_UNSTAGED_BEFORE_FMT value is stale by this point.
if git diff --quiet -- "${MD_FILES[@]}"; then
MD_HAD_UNSTAGED=0
else
MD_HAD_UNSTAGED=1
fi
Comment on lines +167 to +174
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Minor documentation nit: comment reasoning could be clearer.

The comment claims HAD_UNSTAGED_BEFORE_FMT is "stale" after rustfmt auto-staging, but that's not quite accurate:

  • If HAD_UNSTAGED_BEFORE_FMT=0, rustfmt auto-stages and working tree remains clean → not stale.
  • If HAD_UNSTAGED_BEFORE_FMT=1, rustfmt aborts before reaching here → never stale.

The real reason for the re-check is markdown-specific safety: verifying the exact files we're about to auto-stage are clean, independent of the earlier whole-tree snapshot. The logic itself is correct and defensive; just the comment's explanation is slightly off.

📝 Clearer comment wording
-        # Re-snapshot working-tree dirtiness for the markdown path specifically.
-        # The rustfmt step above may have auto-staged its own changes, so the
-        # earlier HAD_UNSTAGED_BEFORE_FMT value is stale by this point.
+        # Check working-tree dirtiness for the markdown files specifically.
+        # Even though we know the whole tree was clean before rustfmt, we verify
+        # these exact files are clean before auto-staging them (defensive check).
         if git diff --quiet -- "${MD_FILES[@]}"; then
📝 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.

Suggested change
# Re-snapshot working-tree dirtiness for the markdown path specifically.
# The rustfmt step above may have auto-staged its own changes, so the
# earlier HAD_UNSTAGED_BEFORE_FMT value is stale by this point.
if git diff --quiet -- "${MD_FILES[@]}"; then
MD_HAD_UNSTAGED=0
else
MD_HAD_UNSTAGED=1
fi
# Check working-tree dirtiness for the markdown files specifically.
# Even though we know the whole tree was clean before rustfmt, we verify
# these exact files are clean before auto-staging them (defensive check).
if git diff --quiet -- "${MD_FILES[@]}"; then
MD_HAD_UNSTAGED=0
else
MD_HAD_UNSTAGED=1
fi
🤖 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 @.githooks/pre-commit around lines 167 - 174, Update the comment above the
markdown re-check to clarify the true reason for re-snapshotting: explain that
we explicitly re-check the cleanliness of the specific MD_FILES to ensure the
exact markdown files we may auto-stage are clean (markdown-specific safety),
rather than saying HAD_UNSTAGED_BEFORE_FMT is simply “stale”; mention the
relevant symbols (MD_FILES, MD_HAD_UNSTAGED, HAD_UNSTAGED_BEFORE_FMT, rustfmt)
so readers know this is a defensive check targeted at markdown auto-staging
rather than a general whole-tree snapshot correction.

# Write changes; suppress stdout but preserve stderr for errors
if ! npx prettier --write "${MD_FILES[@]}" >/dev/null; then
echo "pre-commit: prettier failed" >&2
exit 1
fi
echo "pre-commit: prettier updated markdown files. Aborting commit to preserve index integrity." >&2
echo "Hint: review changes, restage (e.g., 'git add -p' or 'git add -A'), then commit again." >&2
exit 1
if [[ "$MD_HAD_UNSTAGED" -eq 0 ]]; then
echo "pre-commit: prettier updated markdown files; auto-staging (markdown working tree was clean)." >&2
git add -- "${MD_FILES[@]}" || { echo "pre-commit: failed to re-stage prettier changes" >&2; exit 1; }
else
echo "pre-commit: prettier updated markdown files but you had unstaged markdown edits; aborting to preserve partial-staging integrity." >&2
echo "Hint: review changes, restage (e.g., 'git add -p' or 'git add -A'), then commit again." >&2
exit 1
fi
fi
;;
esac
Expand Down
150 changes: 118 additions & 32 deletions crates/echo-wesley-gen/tests/generation.rs
Original file line number Diff line number Diff line change
@@ -1,30 +1,134 @@
// SPDX-License-Identifier: Apache-2.0
// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
#![allow(clippy::unwrap_used, clippy::expect_used)]
#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)]
//! Integration test for the echo-wesley-gen CLI (Wesley IR -> Rust code).

use std::fs;
use std::io::Write;
use std::path::{Path, PathBuf};
use std::process::{Command, Output, Stdio};
use std::sync::OnceLock;

const TOY_COUNTER_IR: &str = include_str!("fixtures/toy-counter/echo-ir-v1.json");

/// Spawns `cargo run -p echo-wesley-gen --`, pipes `ir` to stdin, and returns the output.
// ---------------------------------------------------------------------------
// Test harness speedups.
//
// The integration tests in this file used to be 60-120s each because every
// one of them paid two avoidable costs:
//
// 1. `cargo run -p echo-wesley-gen --` on every invocation, which forced
// cargo to re-check freshness across the whole dependency graph even
// when the binary was already current.
// 2. Each generated consumer crate (under `target/echo-wesley-gen-*-smoke
// /<PID>/...`) had its own independent `target/` directory, so
// `echo-wasm-abi`, `echo-registry-api`, and `serde` were recompiled
// from scratch for every test instead of shared across the suite.
//
// The two helpers below kill both costs:
//
// * `wesley_gen_binary()` builds `echo-wesley-gen` once per test binary
// (`OnceLock`) and returns the path to the compiled binary. All later
// invocations exec the binary directly, skipping cargo's freshness check.
// * `shared_consumer_target_dir()` returns a single stable path under the
// workspace `target/` directory. Every cargo invocation against a
// generated consumer crate runs with `CARGO_TARGET_DIR` set to that
// path, so the heavy upstream dependencies compile exactly once for the
// entire suite and stay cached across `cargo test` runs.
// ---------------------------------------------------------------------------

/// Build `echo-wesley-gen` once per test binary and reuse the compiled path.
///
/// The binary is built under the workspace's normal target directory so the
/// dev profile artifacts are shared with day-to-day workflows. Cargo is
/// still invoked once (the first call) so the binary is guaranteed fresh
/// against the current source; subsequent calls return immediately.
fn wesley_gen_binary() -> &'static Path {
static BINARY: OnceLock<PathBuf> = OnceLock::new();
BINARY
.get_or_init(|| {
// Use `--message-format=json` so we can read the compiler-
// executable path directly instead of guessing where the binary
// landed (works across debug/release and custom profiles).
let output = Command::new("cargo")
.args([
"build",
"-p",
"echo-wesley-gen",
"--bin",
"echo-wesley-gen",
"--message-format=json-render-diagnostics",
])
.stderr(Stdio::inherit())
.output()
.expect("failed to build echo-wesley-gen for tests");
assert!(
output.status.success(),
"cargo build of echo-wesley-gen failed: {}",
String::from_utf8_lossy(&output.stderr)
);
for line in output.stdout.split(|b| *b == b'\n') {
if line.is_empty() {
continue;
}
let msg: serde_json::Value = match serde_json::from_slice(line) {
Ok(v) => v,
Err(_) => continue,
};
if msg.get("reason").and_then(|r| r.as_str()) != Some("compiler-artifact") {
continue;
}
let target = msg.get("target").and_then(|t| t.as_object());
let is_bin = target
.and_then(|t| t.get("kind"))
.and_then(|k| k.as_array())
.is_some_and(|kinds| kinds.iter().any(|k| k.as_str() == Some("bin")));
let name_matches = target.and_then(|t| t.get("name")).and_then(|n| n.as_str())
== Some("echo-wesley-gen");
if !(is_bin && name_matches) {
continue;
}
if let Some(exe) = msg.get("executable").and_then(|e| e.as_str()) {
return PathBuf::from(exe);
}
}
panic!(
"cargo build produced no echo-wesley-gen bin artifact in JSON output:\n{}",
String::from_utf8_lossy(&output.stdout)
);
})
.as_path()
}

/// Returns the shared `CARGO_TARGET_DIR` path used for every generated
/// consumer crate. All consumer crates share this directory so upstream
/// deps (`echo-wasm-abi`, `echo-registry-api`, `serde`, …) compile once
/// for the whole suite instead of once per test, both within a single
/// `cargo test` run and across runs while the path stays warm.
fn shared_consumer_target_dir() -> PathBuf {
let dir = workspace_root()
.join("target")
.join("echo-wesley-gen-test-consumer-cache");
fs::create_dir_all(&dir).expect("failed to create shared consumer target dir");
dir
}

/// Spawns the pre-built `echo-wesley-gen` binary, pipes `ir` to stdin, and
/// returns the output. Equivalent to the old `cargo run -p echo-wesley-gen
/// -- <args>` flow but without re-checking freshness per invocation.
fn run_wesley_gen(ir: &str) -> Output {
run_wesley_gen_with_args(ir, &[])
}

/// Spawns `cargo run -p echo-wesley-gen -- <args>`, pipes `ir` to stdin, and returns the output.
/// Spawns the pre-built `echo-wesley-gen` binary with extra CLI args.
fn run_wesley_gen_with_args(ir: &str, args: &[&str]) -> Output {
let mut child = Command::new("cargo")
.args(["run", "-p", "echo-wesley-gen", "--"])
let mut child = Command::new(wesley_gen_binary())
.args(args)
.stdin(Stdio::piped())
.stdout(Stdio::piped())
.stderr(Stdio::piped())
.spawn()
.expect("failed to spawn cargo run");
.expect("failed to spawn echo-wesley-gen");

let mut stdin = child.stdin.take().expect("failed to get stdin");
stdin
Expand All @@ -36,13 +140,13 @@ fn run_wesley_gen_with_args(ir: &str, args: &[&str]) -> Output {
}

fn run_wesley_gen_schema(schema_path: &Path) -> Output {
Command::new("cargo")
.args(["run", "-p", "echo-wesley-gen", "--", "--schema"])
Command::new(wesley_gen_binary())
.args(["--schema"])
.arg(schema_path)
.stdout(Stdio::piped())
.stderr(Stdio::piped())
.output()
.expect("failed to run cargo run")
.expect("failed to run echo-wesley-gen")
}

fn generated_str_const<'a>(source: &'a str, name: &str) -> &'a str {
Expand Down Expand Up @@ -1283,6 +1387,7 @@ mod tests {

fn assert_generated_crate_checks(crate_dir: &Path) {
let output = Command::new("cargo")
.env("CARGO_TARGET_DIR", shared_consumer_target_dir())
.args(["check", "--manifest-path"])
.arg(crate_dir.join("Cargo.toml"))
.output()
Expand Down Expand Up @@ -1336,6 +1441,7 @@ fn test_reserved_control_op_id_fails_closed() {
fn test_generated_optic_helper_shape_compiles_against_abi() {
let crate_dir = write_optic_binding_smoke_crate();
let output = Command::new("cargo")
.env("CARGO_TARGET_DIR", shared_consumer_target_dir())
.args(["test", "--manifest-path"])
.arg(crate_dir.join("Cargo.toml"))
.output()
Expand Down Expand Up @@ -1748,6 +1854,7 @@ fn test_toy_contract_generated_output_compiles_in_consumer_crate() {
let generated = String::from_utf8_lossy(&output.stdout);
let crate_dir = write_consumer_smoke_crate(&generated);
let output = Command::new("cargo")
.env("CARGO_TARGET_DIR", shared_consumer_target_dir())
.args(["test", "--manifest-path"])
.arg(crate_dir.join("Cargo.toml"))
.output()
Expand Down Expand Up @@ -1777,6 +1884,7 @@ fn test_toy_contract_generated_contract_host_query_observer_compiles_in_consumer
assert!(generated.contains("pub fn counter_value_observer_vars"));
let crate_dir = write_contract_host_smoke_crate(&generated);
let output = Command::new("cargo")
.env("CARGO_TARGET_DIR", shared_consumer_target_dir())
.args(["test", "--manifest-path"])
.arg(crate_dir.join("Cargo.toml"))
.output()
Expand Down Expand Up @@ -1844,29 +1952,7 @@ fn test_generate_no_std_minicbor() {
"ops": []
}"#;

// Run with flags
let mut child = Command::new("cargo")
.args([
"run",
"-p",
"echo-wesley-gen",
"--",
"--no-std",
"--minicbor",
])
.stdin(Stdio::piped())
.stdout(Stdio::piped())
.stderr(Stdio::piped())
.spawn()
.expect("failed to spawn cargo run");

let mut stdin = child.stdin.take().expect("failed to get stdin");
stdin
.write_all(ir.as_bytes())
.expect("failed to write to stdin");
drop(stdin);

let output = child.wait_with_output().expect("failed to wait on child");
let output = run_wesley_gen_with_args(ir, &["--no-std", "--minicbor"]);
let stdout = String::from_utf8_lossy(&output.stdout);
assert!(
output.status.success(),
Expand Down
Loading