Skip to content

Commit 09dff2e

Browse files
committed
ci: enforce the no-panic rule with a script and required CI step
Adds scripts/verify-no-panic-apis.sh and runs it in the check job. Production contract code must return typed errors; a panic in a deployed contract is an untyped failure a caller cannot handle. This converts one rule from enforced-in-review to enforced-mechanically, which is the same alignment the last commit made in the other direction by correcting claims about checks that did not exist. The difference is that this one now runs. Scoping matches the rule rather than approximating it. The script reads each file under src/ up to its first #[cfg(test)] marker, treats a file with no marker as production code throughout, and does not examine tests/ at all. Comments and string literals are stripped before matching, so prose in a doc comment and the message inside .expect("...") are not mistaken for calls. Verified in four directions before wiring: the current tree passes, a planted .unwrap() above a cfg(test) marker is caught with the correct file and line, one planted below it is ignored, and a panic! added to error.rs, which has no cfg(test) block at all, is caught. Banned in production: .unwrap(), .expect(, panic!, todo!, unimplemented!. The failure message names the fix and points at requirements 5.1, CONTRIBUTING, and ADR-015 for the test-code exception. The reference deferred at B2 is now concrete in .agent/testing.md, and requirements 5.1 records that its Rust items are mechanically enforced here.
1 parent edccb0a commit 09dff2e

4 files changed

Lines changed: 111 additions & 3 deletions

File tree

.agent/testing.md

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,14 @@ failed. A bare `.expect("")` or an `unwrap` standing in for an assertion is not
7373
acceptable.
7474

7575
Production code, meaning everything under `src/` outside a `#[cfg(test)]` block,
76-
remains bound by the no-panic rule. See ADR-015. The mechanical check enforcing
77-
it is scoped the same way: it scans each file under `src/` up to the first
78-
`#[cfg(test)]` marker and does not look at `tests/` at all.
76+
remains bound by the no-panic rule. See ADR-015.
77+
78+
`scripts/verify-no-panic-apis.sh` enforces this and runs as a required CI step.
79+
It is scoped the same way the rule is: it reads each file under `src/` up to the
80+
first `#[cfg(test)]` marker, treats a file without one as production code
81+
throughout, and does not look at `tests/` at all. It strips comments and string
82+
literals before matching, so prose in a doc comment and the message inside
83+
`.expect("...")` do not register as calls. Run it locally with no arguments.
7984

8085
## One behavior per test
8186

.github/workflows/ci.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,13 @@ jobs:
7070
if: steps.members.outputs.present == 'true'
7171
run: cargo clippy --workspace --all-targets -- -D warnings
7272

73+
# Production contract code must return typed errors rather than panic.
74+
# The script scopes itself to src/ above each file's first #[cfg(test)],
75+
# so test code keeps the ADR-015 expect exception.
76+
- name: Verify no panicking APIs in production code
77+
if: steps.members.outputs.present == 'true'
78+
run: ./scripts/verify-no-panic-apis.sh
79+
7380
- name: Test
7481
if: steps.members.outputs.present == 'true'
7582
run: cargo test --workspace

docs/requirements.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -426,6 +426,13 @@ Reinforcing the playbook's operating principles. These rules apply to every arti
426426
- No `unimplemented!()` (Rust)
427427
- No `panic("not implemented")` (Go)
428428
- No `throw new Error("TODO")` (TS)
429+
430+
In `pulsar-core` the Rust items are enforced mechanically by
431+
`scripts/verify-no-panic-apis.sh`, a required CI step. It also covers `.unwrap()`,
432+
`.expect(`, `panic!`, and `todo!`, which CONTRIBUTING forbids in contract code for
433+
the same reason: a panic is an untyped failure a caller cannot handle. The check
434+
reads each file under `src/` up to its first `#[cfg(test)]` marker, so test code
435+
keeps the descriptive-message exception in ADR-015.
429436
- Every function commit is complete and testable at the moment it lands
430437

431438
### 5.2 No fabricated numbers

scripts/verify-no-panic-apis.sh

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
#!/usr/bin/env bash
2+
#
3+
# Fails if production contract code uses an API that panics instead of returning
4+
# a typed error. A panic in a deployed contract is an untyped failure a caller
5+
# cannot handle, which is why requirements.md section 5.1 forbids the placeholder
6+
# forms and CONTRIBUTING forbids the rest.
7+
#
8+
# Scope: everything under a crate's src/ directory, up to the first #[cfg(test)]
9+
# marker in each file. Tests below that marker are exempt, and integration tests
10+
# under tests/ are not examined at all. Test code may use .expect("message") with
11+
# a descriptive message, per ADR-015: there the panic text is the diagnostic.
12+
#
13+
# Usage: scripts/verify-no-panic-apis.sh [src-dir ...]
14+
# Defaults to every contracts/*/src and crates/*/src directory that exists.
15+
16+
set -euo pipefail
17+
18+
BANNED_DESC=(
19+
'.unwrap()'
20+
'.expect('
21+
'panic!'
22+
'todo!'
23+
'unimplemented!'
24+
)
25+
# Matched against code with comments and string literals already stripped.
26+
BANNED_RE='\.unwrap\(\)|\.expect\(|\bpanic!|\btodo!|\bunimplemented!'
27+
28+
if [ "$#" -gt 0 ]; then
29+
search_dirs=("$@")
30+
else
31+
search_dirs=()
32+
for d in contracts/*/src crates/*/src; do
33+
[ -d "$d" ] && search_dirs+=("$d")
34+
done
35+
fi
36+
37+
if [ "${#search_dirs[@]}" -eq 0 ]; then
38+
echo "No source directories found. Nothing to check."
39+
exit 0
40+
fi
41+
42+
violations=0
43+
files_checked=0
44+
45+
while IFS= read -r file; do
46+
files_checked=$((files_checked + 1))
47+
48+
# Production code is everything above the first #[cfg(test)]. A file without
49+
# one is production code throughout.
50+
cutoff=$(grep -n '#\[cfg(test)\]' "$file" | head -1 | cut -d: -f1 || true)
51+
if [ -n "$cutoff" ]; then
52+
end=$((cutoff - 1))
53+
else
54+
end=$(wc -l < "$file")
55+
fi
56+
[ "$end" -lt 1 ] && continue
57+
58+
# Strip line comments and string literals before matching, so prose in a doc
59+
# comment and a message inside .expect("...") do not register as calls.
60+
hits=$(
61+
head -n "$end" "$file" \
62+
| sed -e 's://.*::' -e 's:"[^"]*":"":g' \
63+
| grep -nE "$BANNED_RE" \
64+
|| true
65+
)
66+
67+
if [ -n "$hits" ]; then
68+
while IFS= read -r hit; do
69+
lineno=${hit%%:*}
70+
printf '%s:%s: %s\n' "$file" "$lineno" \
71+
"$(sed -n "${lineno}p" "$file" | sed 's/^[[:space:]]*//')"
72+
violations=$((violations + 1))
73+
done <<< "$hits"
74+
fi
75+
done < <(find "${search_dirs[@]}" -name '*.rs' -type f | sort)
76+
77+
if [ "$violations" -gt 0 ]; then
78+
echo
79+
echo "Found $violations panicking API call(s) in production code."
80+
echo
81+
echo "Banned in production: ${BANNED_DESC[*]}"
82+
echo
83+
echo "Convert Option to Result with .ok_or(Error::Variant) and propagate with ?."
84+
echo "See requirements.md section 5.1, CONTRIBUTING.md code rules, and ADR-015"
85+
echo "for the test-code exception."
86+
exit 1
87+
fi
88+
89+
echo "No panicking APIs in production code. Checked $files_checked file(s)."

0 commit comments

Comments
 (0)