fix: exit-code contract, packaging and source polish#15
Conversation
The composite action and CI examples pinned prview 0.6.0, which is not published on crates.io (latest is 0.5.0) and has no matching git tag, so any copy-pasted CI setup hard-failed at install resolution. Default the action to a resilient "latest" and reference @main for the action, since the gate command and the composite action itself post-date the 0.5.0 tag. Documents honestly that gate is not yet in a published crate release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
clap exits 2 on a CLI usage error (unknown flag in `args`), the same code the gate contract uses for a strict CONDITIONAL. The action mapped both to "CONDITIONAL rejected by strict mode", turning an argument typo into a phantom gate verdict. A real verdict always writes its gate JSON to stdout; a usage error writes nothing there. Gate the exit-2 branch on that sentinel and report usage errors as an execution error (exit 3) with a clear message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
"${arr[@]}" on an empty array under `set -u` raises "unbound variable" on
bash < 4.4 (macOS ships 3.2). The install step hit this whenever version was
"latest" (empty CARGO_VERSION_ARGS). Use the ${arr[@]+"${arr[@]}"} idiom for
both the cargo install version args and the passthrough gate args.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The shadow gate workflow wrote and read the gate JSON via bare $RUNNER_TEMP,
while the composite action already falls back to ${RUNNER_TEMP:-/tmp} (7195478).
Align the shadow workflow so a local/self-hosted run without RUNNER_TEMP writes
to /tmp instead of the filesystem root, and route every read through the same
resolved path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The exit-code mapping was only covered by a pure-function unit test in src/gate.rs; nothing exercised the actual process exit codes the composite action keys pass/fail on. Add an e2e test that drives the binary for every contract branch: 0 (non-strict CONDITIONAL), 2 (strict CONDITIONAL), 1 (BLOCK via a default_severity: block policy), and 3 (gate cannot execute outside a git repo). Deterministic without depending on installed quality tools, since the gate profile always skips heuristics_loctree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Remove the raw combined-output warning substring fallback from semgrep status classification. - Keep structured JSON errors[] as the downgrade source for partial parsing and scan errors. - Add a regression for clean semgrep JSON with a warning-named scanned path.
- Declare the crate MSRV in Cargo.toml so older cargo install attempts fail clearly. - Use the lowest locally verified toolchain for the current locked dependency graph: Rust 1.93.0. - Keep CI's newer 1.94 pin unchanged.
- Normalize absolute external paths before formatting their display string. - Emit the same [external]/ prefix shape for relative escapes and absolute out-of-repo paths. - Tighten path tests around exact prefix stripping and cleaned absolute paths.
- Treat missing prview.toml as the only quiet None path. - Emit a warning when prview.toml exists but cannot be read. - Add subprocess-backed tests for quiet NotFound and visible read-error stderr.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the GitHub Action and documentation to support the prview gate command, defaults the installation version to latest, handles exit code 2 ambiguity between CLI usage errors and strict CONDITIONAL verdicts, refactors semgrep status classification, improves prview.toml read error handling, and adds end-to-end integration tests for gate exit codes. The reviewer feedback suggests simplifying the shell check for the gate verdict, refactoring manifest loading to return a Result to avoid fragile process-spawning unit tests, and making the new integration tests more robust by disabling GPG signing and setting GIT_CEILING_DIRECTORIES in the test git repositories.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # error writes nothing to stdout (clap prints to stderr). Use that | ||
| # sentinel so an argument typo surfaces as a usage error, not a | ||
| # phantom "CONDITIONAL rejected" verdict. | ||
| if [ "$verdict" = "CONDITIONAL" ] || { [ -z "$verdict" ] && [ -s "$JSON_PATH" ]; }; then |
There was a problem hiding this comment.
Since any successful execution of prview gate redirects its JSON output to $JSON_PATH (making it non-empty), and a CLI usage error from clap only writes to stderr (leaving $JSON_PATH empty), checking [ -s "$JSON_PATH" ] is a sufficient and highly robust sentinel. This avoids complex conditional logic and potential issues with jq parsing or unexpected verdict values.
if [ -s "$JSON_PATH" ]; thenThere was a problem hiding this comment.
Fixed in 0285347. The action now treats exit code 2 as a strict CONDITIONAL verdict only when the gate JSON sentinel file is non-empty; otherwise it fails as a usage error. This preserves the 348fe37 contract that successful gate verdicts emit JSON, while avoiding verdict inference from parsed fields.
| let output = std::process::Command::new(std::env::current_exe().unwrap()) | ||
| .args([ | ||
| "--exact", | ||
| "config::manifest::tests::test_load_missing_file_returns_none", | ||
| "--nocapture", | ||
| ]) | ||
| .env(CHILD_ENV, "1") | ||
| .output() | ||
| .unwrap(); |
There was a problem hiding this comment.
Spawning std::env::current_exe() in unit tests to assert on eprintln! output is fragile, slow, and can fail in cross-compilation environments or under alternative test runners (like cargo-nextest).
A more robust and idiomatic Rust approach is to refactor load_from to return a Result<Self, ManifestError> (or accept a generic writer/logger for warnings). This allows you to unit-test all success and failure cases (including missing files, read errors, and parse errors) cleanly and quickly without process spawning or stderr capturing.
There was a problem hiding this comment.
Fixed in c77d379. The manifest warning tests no longer spawn std::env::current_exe(); warning emission is routed through a private sink helper and tested directly. Public load_from() behavior is unchanged, including the existing stderr warning path.
| run_git(repo, &["init"]); | ||
| run_git(repo, &["config", "user.name", "Test User"]); | ||
| run_git(repo, &["config", "user.email", "test@example.com"]); |
There was a problem hiding this comment.
If a developer has global GPG signing enabled (commit.gpgsign = true) in their git configuration, running these tests locally will fail because git commit will attempt to sign the commits and fail.
To make the test suite hermetic and robust across different developer environments, disable GPG signing locally in the test repository.
| run_git(repo, &["init"]); | |
| run_git(repo, &["config", "user.name", "Test User"]); | |
| run_git(repo, &["config", "user.email", "test@example.com"]); | |
| run_git(repo, &["init"]); | |
| run_git(repo, &["config", "user.name", "Test User"]); | |
| run_git(repo, &["config", "user.email", "test@example.com"]); | |
| run_git(repo, &["config", "commit.gpgsign", "false"]); |
There was a problem hiding this comment.
Fixed in 4ba4787. The git fixture helper now disables commit.gpgsign and tag.gpgsign via per-command git -c config, so a developer's global signing setup cannot break fixture commits. Verified with a hostile smoke using GIT_CONFIG_GLOBAL that enables both signing options.
| Command::new(assert_cmd::cargo::cargo_bin!("prview")) | ||
| .current_dir(temp.path()) | ||
| .arg("gate") | ||
| .assert() | ||
| .code(3); |
There was a problem hiding this comment.
If the system's temporary directory is located within a parent git repository (which is common in some CI environments or nested workspace checkouts), git will walk up the directory tree and discover the parent repository. This will cause prview gate to run against the parent repository instead of failing with exit code 3, making the test flaky or failing.
To ensure the test reliably runs outside of any git repository, you can set the GIT_CEILING_DIRECTORIES environment variable to the temporary directory's path to prevent git from searching upwards.
Command::new(assert_cmd::cargo::cargo_bin!("prview"))
.current_dir(temp.path())
.env("GIT_CEILING_DIRECTORIES", temp.path())
.arg("gate")
.assert()
.code(3);There was a problem hiding this comment.
Fixed in 1c56cdf. The exit-three fixture now sets GIT_CEILING_DIRECTORIES to the fixture temp root, keeping repository discovery from climbing into a parent checkout. Verified with TMPDIR nested inside a temporary parent git repository.
![chatgpt-codex-connector[bot]](https://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: 8aaae5a0fd
ℹ️ 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".
| .current_dir(temp.path()) | ||
| .args(["gate", "--strict"]) | ||
| .assert() | ||
| .code(2); |
There was a problem hiding this comment.
When semgrep is installed on the runner, this fixture is not guaranteed to be a CONDITIONAL run: the Generic profile still includes SemgrepCheck (src/checks/mod.rs) and that check only skips when the binary is absent (src/checks/semgrep.rs), while the synthetic heuristics skip does not by itself drive the merge verdict. In that environment the gate can return PASS/exit 0 here instead of 2, and the block-policy test below can similarly miss exit 1; make the fixture force a deterministic skipped check or otherwise isolate semgrep from PATH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ba70aab. The gate exit-code tests now run prview gate with a fixture PATH that exposes git but intentionally hides semgrep, making the Semgrep scan deterministically skipped. Verified on a host where semgrep is actually installed.
| that does not yet exist on crates.io. | ||
| required: false | ||
| default: "0.6.0" | ||
| default: "latest" |
There was a problem hiding this comment.
For consumers following the new @main action snippet before a gate-capable crate is published, this default takes the install path through cargo install prview --locked --force (Cargo help: cargo install [CRATE[@<VER>]] selects a package from a source unless --git/--path is used). The docs in this diff state prview gate is not in the 0.5.0 crate, so version: latest can install a binary that lacks the gate subcommand and the action then fails as a usage error; default to a known gate-capable version or install from this checkout until such a release exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 991dc39. After installation, the action now probes prview gate --help and exits 3 with a clear message if the installed binary has no gate subcommand. Simulated a prview 0.5.0 binary and confirmed the action fails fast with an explicit message instead of a cryptic error.
- Simplify the exit-2 disambiguation to the stdout JSON sentinel. - Preserve clap usage errors as action exit 3 when no gate JSON is emitted. - Avoid coupling the verdict branch to jq parsing or future verdict strings.
- Add a private warning sink around manifest loading. - Keep the public load_from contract unchanged. - Assert missing/read-error warning behavior directly in unit tests.
- Run fixture git commands with commit and tag signing disabled. - Keep developer global gpgsign settings from breaking test commits. - Leave the gate exit-code contract unchanged.
- Set GIT_CEILING_DIRECTORIES for the non-repository gate run. - Prevent git discovery from walking into a parent checkout. - Preserve the execution-error exit-code assertion.
- Run gate fixture commands with a PATH that exposes git but not semgrep. - Make CONDITIONAL and BLOCK assertions independent of runner tooling. - Verify the gate exit-code suite passes with semgrep installed locally.
- Probe prview gate --help after either install method. - Report a clear pre-gate crate message instead of a later usage error. - Keep the latest default and release decision unchanged.
1 participant
Summary
Contract & packaging round — companion to #14, closing the remaining confirmed findings from the 2026-07-07 deep-dig audit (both packaging P1/P2) plus targeted source polish. Fully disjoint from #14's file set; the two PRs merge independently.
Install & exit-code contract
a3e8a71fix(docs): point install/action defaults at released version — README/action.yml/docs pinned v0.6.0, which was never released (no tag, crates.io at 0.5.0): anyone copying the documented CI setup got a hard install failure. Defaults now resilient (latest/@main), all dead-version references removed.348fe37fix(action): distinguish usage error from conditional verdict — the composite gate action mapped clap's exit 2 (usage error) to "CONDITIONAL rejected by strict mode", so a flag typo produced a fake verdict instead of an error. The action now detects a gate-JSON sentinel on stdout before trusting exit codes.f22bbe3test(gate): add end-to-end exit-code contract test — the 0/1/2/3 gate contract now has binary-level e2e coverage (tests/gate_exit_codes.rs).28a1687fix(action): bash 3.2 safe array expansion (macOS default bash breaks on empty arrays underset -u).40f3f46fix(ci): default RUNNER_TEMP in shadow gate workflow (parity with the composite action).Source polish
13f6e43fix(checks): stop degrading clean semgrep scan on warning substring — a literal "warning" in a file path or rule note no longer flips a clean scan to Warnings.af5a2e7chore: declare rust-version (MSRV) —rust-version = "1.93.0", verified against the Rust 2024 edition guide and cross-checked locally (1.85.0 fails on the current dependency graph, 1.93.0 passes).2e6cad8fix(paths): unify[external]/prefix across branches — the absolute-out-of-repo branch emitted[external]<path>without a separator while the relative-escape branch emitted[external]/<path>.8aaae5afix(config): warn on manifest read errors other than not-found — permission errors on.prview.tomlno longer vanish silently.Known limitation (operator decision, out of scope here)
The gate feature itself (
src/gate.rs,action.yml) exists in no released artifact —v0.5.0predates it andv0.6.0was never tagged. This PR makes the docs honest and resilient, but external consumers get a working gate only after a 0.6.0 release (deliberately NOT performed in this round).Test plan
cargo fmt --check,cargo clippy --all-targets -- -D warnings,cargo testgreen (1250 passed / 0 failed); shellcheck clean on all touched bash.[external]/round-trip, and manifest read-error warning.🤖 Generated with Claude Code