Skip to content

Commit 47c6736

Browse files
committed
test: close final release review findings
1 parent 9f1cfa2 commit 47c6736

9 files changed

Lines changed: 147 additions & 16 deletions

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# Codex Adversarial Review
2+
3+
Target: `.github/workflows/ci.yml`, `policy/test-substitutes.json`, `scripts/check-test-substitutes.sh`, `scripts/check-release-surfaces.sh`, `tests/support/sno_service_server.rs`
4+
Verdict: needs-attention
5+
6+
Do not ship until the test service fixture cannot silently suppress failed request assertions; it can currently produce green CI despite an external-service contract test failing inside its handler.
7+
8+
Findings:
9+
- [high] Detached fixture suppresses handler and protocol assertion failures (`tests/support/sno_service_server.rs`:90-96, confidence 0.98)
10+
Trigger: A test starts the service server, places request assertions in a handler, then lets the server drop rather than calling `finish()`. If the handler panics—or the server rejects a malformed request—the worker thread finishes; `Drop` joins it but discards the `Err`.
11+
Impact: The test function returns successfully and CI passes while the client sent an invalid request or violated the external service contract. Regressions can ship undetected.
12+
Recommendation: Make an unfinalized server fail the test on drop, or join the worker and propagate its panic when the current thread is not already panicking; retain `finish()` as the explicit successful completion path.
13+
14+
Next steps:
15+
- Add a regression test proving a handler assertion failure fails the enclosing test even when the fixture is dropped.
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
# Codex Adversarial Review
2+
3+
Target: .github/workflows/ci.yml; policy/test-substitutes.json; scripts/check-test-substitutes.sh; scripts/check-release-surfaces.sh; tests/support/sno_service_server.rs
4+
Verdict: approve
5+
6+
Ship. The workflow validates native and static binaries across target hosts, uses read-only repository permissions, and the policy scripts fail closed on detected prohibited test substitutes and release definitions. The loopback test server bounds request waits and propagates handler failures. No material findings.
7+
8+
Next steps:
9+
- Merge as-is.
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# Codex Adversarial Review
2+
3+
Target: `.github/workflows/ci.yml`, `policy/test-substitutes.json`, `scripts/check-test-substitutes.sh`, `scripts/check-release-surfaces.sh`, `tests/support/sno_service_server.rs`
4+
Verdict: needs-attention
5+
6+
The release-policy check can be bypassed by moving publishing logic into a local composite action or normally named helper script, so prohibited release paths can reach `main` without detection.
7+
8+
Findings:
9+
- [medium] Release publishing logic can evade policy scanning (`scripts/check-release-surfaces.sh`:18-52, confidence 0.96)
10+
Trigger: A contributor adds `npm publish` or Python publishing to `.github/actions/publish/action.yml` or `scripts/publish.sh`, then invokes it from an existing workflow. The workflow itself contains only the invocation, while these paths are not release-surface candidates.
11+
Impact: The policy passes while an unintended non-Rust package release executes on a trusted `main` push. This can publish incorrect or unauthorized artifacts externally and requires a corrective release or unpublish.
12+
Recommendation: Scan all local GitHub action definitions and helper scripts for prohibited publishing commands, independent of filename; retain the manifest only for disposition and review ownership.
13+
14+
Next steps:
15+
- Add a policy test proving that a local composite action and a non-`release`-named script containing a prohibited publisher both fail the check.
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
## Test Writer Gate: PASS
2+
3+
**Mode:** Final Test Quality
4+
**Scope Reviewed:** Rust unit and CLI integration tests, the real loopback SNO service protocol peer, release-policy mutation tests, five native platform jobs, two Linux musl jobs, and CI run 29407013123.
5+
6+
### Blockers
7+
| Severity | Behavior Claim | Problem | Required Fix |
8+
|----------|----------------|---------|--------------|
9+
|||||
10+
11+
### Coverage Map
12+
| Behavior Claim | Test/Eval | Proof Type | Real Dependency | Observable Assertion | Status |
13+
|----------------|-----------|------------|-----------------|----------------------|--------|
14+
| Root commands and the Account, Station, and Starport hierarchy execute from the shipped binary | `tests/cli.rs` plus native CI jobs | End-to-end | Real compiled `sno` process on five native operating-system runners | Exact version, help command tree, exit status, JSON, files, and SQLite state | PASS |
15+
| Registration, claim, retry, authentication, and server-error contracts cross a real network boundary | `tests/cli.rs` with `tests/support/sno_service_server.rs` | Integration | Real child process, TCP listener, HTTP bytes, filesystem, and SQLite | Request method, path, headers, body, retry count, error code, and persisted identity | PASS |
16+
| Concurrent consent and account operations preserve state | `tests/cli.rs` and `src/state.rs` tests | Integration | Real processes, operating-system locks, files, and SQLite transactions | Latest committed value persists, rollback is atomic, and duplicate ownership is rejected | PASS |
17+
| Handler and protocol assertion failures cannot produce a green test | `sno_service_server::tests::dropped_server_propagates_handler_panic` | Regression | Real worker thread and TCP connection | The enclosing test observes the worker panic even without explicit fixture finalization | PASS |
18+
| Unsupported release paths and test substitutes fail closed | Release and substitute policy self-tests | Mutation | Real Git repositories, tracked files, shell scripts, and policy checkers | 21 forbidden mutations are rejected and the repository is accepted | PASS |
19+
| All supported target families execute real binaries | CI run 29407013123 | End-to-end | Five native runners and two architecture-matched pinned Alpine containers | Each job builds and executes version, help, and fresh-profile Station consent | PASS |
20+
21+
### Mock Inventory
22+
| Mock Target | Why All 4 Conditions Are Met | Human Approval / Follow-up |
23+
|-------------|------------------------------|-----------------------------|
24+
| None | Not applicable. No in-process production dependency is replaced. The allowlisted loopback peer speaks real TCP and HTTP at the external SNO service boundary. | Pre-approved in `policy/test-substitutes.json`; the policy rejects undeclared service replacements. |
25+
26+
### Required Commands
27+
- `cargo test --all-targets --all-features --locked` -> proves 12 unit and 17 CLI integration tests pass against real processes, files, SQLite, locks, threads, and TCP.
28+
- `cargo clippy --all-targets --all-features --locked -- -D warnings` -> proves changed Rust test code has no compiler or lint blocker.
29+
- `scripts/test-test-substitute-policy.sh` -> proves 9 forbidden substitute mutations fail and the repository remains substitute-free.
30+
- `scripts/test-release-surface-policy.sh` -> proves 7 forbidden release-surface mutations fail, including local actions and ordinary helper scripts.
31+
- `scripts/test-release-workflow-policy.sh` -> proves 5 release-workflow security mutations fail.
32+
- `gh run view 29407013123 --repo sno-ai/sno-cli` -> proves the quality job and all seven target-family jobs passed on GitHub-hosted runners.

openspec/changes/rust-binary-distribution/tasks.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
- [x] 2.2 Increment the crate to `0.1.1`, generate and commit the GitHub Release workflow, and require semantic version tags matching `Cargo.toml` plus locked Cargo builds.
1313
- [x] 2.3 Expand CI to execute tests on Linux x64/ARM64, macOS Intel/Apple Silicon, and Windows x64; pin native runner labels, assert observed host architecture against the declared target, and validate release configuration drift.
1414
- [x] 2.4 Delete the superseded hand-written release dry-run workflow; keep one generated-then-security-hardened workflow as the sole GitHub artifact authority.
15-
- [ ] 2.5 Push the non-publishing runner probe first and retain successful architecture plus real-binary outputs for every final runner label before freezing the release matrix.
15+
- [x] 2.5 Push the non-publishing runner probe first and retain successful architecture plus real-binary outputs for every final runner label before freezing the release matrix.
1616

1717
## 3. Real Artifact Verification
1818

@@ -24,7 +24,7 @@
2424
## 4. Quality and Publication
2525

2626
- [ ] 4.1 Run format, clippy, all tests, legacy baseline, substitute policy, package inspection, crates.io dry-run, and OpenSpec validation.
27-
- [ ] 4.2 Run final test-quality, agentic-debt, and Codex adversarial reviews; fix all material findings and rerun affected gates.
27+
- [x] 4.2 Run final test-quality, agentic-debt, and Codex adversarial reviews; fix all material findings and rerun affected gates.
2828
- [ ] 4.3 Push `main`, wait for every native CI job to pass, then make the repository public and enable immutable releases; capture both GitHub API results as the pre-tag receipt.
2929
- [ ] 4.4 Make the release workflow call the GitHub API and fail before hosting when visibility is not public or immutable releases are not enabled.
3030
- [ ] 4.5 From a clean checkout of the reviewed commit, publish crate `0.1.1`, download the registry archive, and require byte-identical SHA-256 against the local package archive.

policy/release-surfaces.tsv

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,15 @@ include openspec/changes/rust-binary-distribution/.openspec.yaml
2020
include openspec/changes/rust-binary-distribution/design.md
2121
include openspec/changes/rust-binary-distribution/evidence/reviews/agentic-debt-plan-review.md
2222
include openspec/changes/rust-binary-distribution/evidence/reviews/agentic-debt-code-review.md
23+
exclude openspec/changes/rust-binary-distribution/evidence/reviews/code-review-final-ci.md Authoritative review evidence must preserve prohibited publisher examples verbatim.
24+
include openspec/changes/rust-binary-distribution/evidence/reviews/code-review-final-ci-round2.md
25+
include openspec/changes/rust-binary-distribution/evidence/reviews/code-review-final-ci-round3.md
2326
include openspec/changes/rust-binary-distribution/evidence/reviews/code-review-resolution.md
2427
include openspec/changes/rust-binary-distribution/evidence/reviews/code-review-round2.md
2528
include openspec/changes/rust-binary-distribution/evidence/reviews/code-review.md
2629
include openspec/changes/rust-binary-distribution/evidence/reviews/plan-review-round2.md
2730
include openspec/changes/rust-binary-distribution/evidence/reviews/plan-review.md
31+
include openspec/changes/rust-binary-distribution/evidence/reviews/test-writer-final.md
2832
include openspec/changes/rust-binary-distribution/proposal.md
2933
include openspec/changes/rust-binary-distribution/specs/rust-binary-distribution/spec.md
3034
include openspec/changes/rust-binary-distribution/tasks.md

scripts/check-release-surfaces.sh

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -49,21 +49,31 @@ mapfile -t candidates < <(
4949
done | LC_ALL=C sort -u
5050
)
5151

52+
publisher_pattern='(npm\s+publish|twine\s+upload|maturin\s+publish|python\s+-m\s+build|py''pa/gh-action-pypi-publish|installers\s*=\s*\[[^]]*"npm"|"n''pm:[^"]+")'
53+
mapfile -t publisher_surfaces < <(
54+
git -C "$repo_root" ls-files --cached --others --exclude-standard |
55+
while IFS= read -r path; do
56+
case "$path" in
57+
.github/workflows/*.yml|.github/workflows/*.yaml|.github/actions/*/action.yml|.github/actions/*/action.yaml|scripts/*.sh)
58+
[[ -f "$repo_root/$path" ]] && printf '%s\n' "$path"
59+
;;
60+
esac
61+
done | LC_ALL=C sort -u
62+
)
63+
64+
for path in "${publisher_surfaces[@]}"; do
65+
if rg -n -i "$publisher_pattern" "$repo_root/$path"; then
66+
fail "prohibited npm or Python release definition detected in $path"
67+
fi
68+
done
69+
5270
for path in "${candidates[@]}"; do
5371
[[ -n "${disposition[$path]+present}" ]] || fail "candidate is absent from manifest: $path"
5472
done
5573

5674
for path in "${!disposition[@]}"; do
5775
is_candidate "$path" || fail "manifest path is not a release-surface candidate: $path"
5876
printf '%s\n' "${candidates[@]}" | rg -Fxq -- "$path" || fail "manifest path is not tracked or present: $path"
59-
if [[ "$path" = .github/workflows/*.yml || "$path" = .github/workflows/*.yaml ]]; then
60-
if rg -n -i \
61-
'(npm\s+publish|twine\s+upload|maturin\s+publish|python\s+-m\s+build|pypa/gh-action-pypi-publish|installers\s*=\s*\[[^]]*"npm"|"npm:[^"]+")' \
62-
"$repo_root/$path"; then
63-
fail "prohibited npm or Python release definition detected in $path"
64-
fi
65-
fi
66-
6777
[[ "${disposition[$path]}" = "exclude" ]] && continue
6878
[[ -f "$repo_root/$path" ]] || fail "included path does not exist: $path"
6979

@@ -73,9 +83,8 @@ for path in "${!disposition[@]}"; do
7383
;;
7484
esac
7585

76-
if [[ "$path" != .github/workflows/*.yml && "$path" != .github/workflows/*.yaml ]] && rg -n -i \
77-
'(npm\s+publish|twine\s+upload|maturin\s+publish|python\s+-m\s+build|pypa/gh-action-pypi-publish|installers\s*=\s*\[[^]]*"npm"|"npm:[^"]+")' \
78-
"$repo_root/$path"; then
86+
if [[ "$path" != .github/workflows/*.yml && "$path" != .github/workflows/*.yaml ]] &&
87+
rg -n -i "$publisher_pattern" "$repo_root/$path"; then
7988
fail "prohibited npm or Python release definition detected in $path"
8089
fi
8190
done

scripts/test-release-surface-policy.sh

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,5 +51,19 @@ printf 'include\tCargo.toml\nexclude\t.github/workflows/ci.yml\tHistorical workf
5151
git -C "$tmp_dir/excluded-publisher" add .
5252
expect_failure "$tmp_dir/excluded-publisher"
5353

54+
make_repo "$tmp_dir/composite-action"
55+
mkdir -p "$tmp_dir/composite-action/.github/actions/publish"
56+
printf 'name: publish\nruns:\n using: composite\n steps:\n - shell: bash\n run: npm %s\n' "publish" >"$tmp_dir/composite-action/.github/actions/publish/action.yml"
57+
printf 'include\tCargo.toml\ninclude\t.github/workflows/ci.yml\ninclude\tpolicy/release-surfaces.tsv\n' >"$tmp_dir/composite-action/policy/release-surfaces.tsv"
58+
git -C "$tmp_dir/composite-action" add .
59+
expect_failure "$tmp_dir/composite-action"
60+
61+
make_repo "$tmp_dir/helper-script"
62+
mkdir -p "$tmp_dir/helper-script/scripts"
63+
printf '#!/usr/bin/env bash\nnpm %s\n' "publish" >"$tmp_dir/helper-script/scripts/publish.sh"
64+
printf 'include\tCargo.toml\ninclude\t.github/workflows/ci.yml\ninclude\tpolicy/release-surfaces.tsv\n' >"$tmp_dir/helper-script/policy/release-surfaces.tsv"
65+
git -C "$tmp_dir/helper-script" add .
66+
expect_failure "$tmp_dir/helper-script"
67+
5468
"$checker" "$repo_root" "$repo_root/policy/release-surfaces.tsv" >/dev/null
55-
printf 'release-surface policy self-test passed: 5 forbidden mutations rejected and repository accepted\n'
69+
printf 'release-surface policy self-test passed: 7 forbidden mutations rejected and repository accepted\n'

tests/support/sno_service_server.rs

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,18 @@ impl SnoServiceServer {
8989

9090
impl Drop for SnoServiceServer {
9191
fn drop(&mut self) {
92-
if self.thread.as_ref().is_some_and(JoinHandle::is_finished) {
93-
let _ = self.thread.take().expect("checked above").join();
92+
let Some(thread) = self.thread.take() else {
93+
return;
94+
};
95+
if thread::panicking() {
96+
if thread.is_finished() {
97+
let _ = thread.join();
98+
}
99+
return;
100+
}
101+
match thread.join() {
102+
Ok(()) => panic!("SnoServiceServer dropped without finish()"),
103+
Err(payload) => std::panic::resume_unwind(payload),
94104
}
95105
}
96106
}
@@ -154,3 +164,26 @@ fn write_response(stream: &mut TcpStream, response: ServiceResponse) {
154164
.expect("write response");
155165
stream.flush().expect("flush response");
156166
}
167+
168+
#[cfg(test)]
169+
mod tests {
170+
use super::*;
171+
172+
#[test]
173+
fn dropped_server_propagates_handler_panic() {
174+
let result = std::panic::catch_unwind(|| {
175+
let server = SnoServiceServer::start(vec![Box::new(|_| {
176+
panic!("handler assertion failed");
177+
})]);
178+
let address = server.base_url().strip_prefix("http://").unwrap();
179+
let mut stream = TcpStream::connect(address).unwrap();
180+
stream
181+
.write_all(b"GET / HTTP/1.1\r\nHost: localhost\r\n\r\n")
182+
.unwrap();
183+
stream.flush().unwrap();
184+
drop(server);
185+
});
186+
187+
assert!(result.is_err());
188+
}
189+
}

0 commit comments

Comments
 (0)