test: add F1/F2/F3/F7 regression coverage from audit sha e289021#83
Merged
Conversation
PR #82 fixed all 7 audit findings but only F5 shipped with an automated regression test (TAP T78/T79); F1/F2/F3/F4/F6/F7 relied on one-time manual verification described in commit messages, leaving them free to silently regress on the next change to ci-deep.yml/ci-build.sh/README.md. Adds three new tools/test_*.sh scripts, wired into build-test.yml's validation job (no nginx build required): - test_fuzz_seconds_validation.sh (F1): drives the exact validation shape from ci-deep.yml's "Select fuzz duration" step against injection payloads, malformed input, and the 14400s boundary. - test_ci_build_provenance.sh (F2+F3): reproduces ci-build.sh's extract+guarded-mv sequence against a tarball whose top-level dir name collides with SRCDIR (the pre-fix failure shape), plus wrong-sha256 and corrupted-cache-hit rejection cases. Includes one live clean-root angie build (skipped offline). - test_docs_ci_drift.sh (F7): checks every .github/workflows/*.yml filename is referenced in README.md and vice versa, that SECURITY.md doesn't link the removed AGENTS.md, and that CONTRIBUTING.md's fuzz -Werror claim matches reality. Caught a real drift on first run -- bump.yml existed but was never referenced by filename in README.md; fixed by adding a Bump row to the workflow table. F4 (bump.yml's PR-open path) and F6 (soak.sh's failure detection) are left as accepted gaps: F4 needs a live protected-branch fixture repo to test meaningfully, and F6's own fix already runs as the regression guard in every existing soak invocation. All three scripts pass shellcheck/shfmt -i 4 -ci/actionlint clean.
📝 WalkthroughWalkthroughAdds CI regression scripts for fuzz-duration validation, documentation/workflow drift, and ChangesCI regression audits
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@tools/test_docs_ci_drift.sh`:
- Around line 60-65: Update the -Werror validation in the drift check around the
grep conditions so workflow matches are restricted to the fuzz build step or
fuzz-specific workflow, rather than any .github workflow containing -Werror.
Preserve the existing direct fuzz/ source check and failure message, ensuring
unrelated workflow flags cannot satisfy the test.
In `@tools/test_fuzz_seconds_validation.sh`:
- Around line 86-97: The workflow validation test must verify the env:
RAW_FUZZ_SECONDS handoff rather than only the copied guard and budget patterns;
update tools/test_fuzz_seconds_validation.sh at lines 86-97 to assert that
wiring. Strengthen the offline F2 coverage in tools/test_ci_build_provenance.sh
at lines 104-110 by executing the real tools/ci-build.sh with a fixture-based
offline case instead of only replaying equivalent local logic.
- Around line 29-37: Update the validation logic around RAW_FUZZ_SECONDS in
validate to reject digit strings exceeding the supported range before invoking
the [ numeric comparison, preventing arithmetic errors from reaching the
FUZZ_SECS output. Add a regression case in the test script using an oversized
all-digit input and verify validation fails without emitting FUZZ_SECS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4c258cd-3eec-4a75-90ec-541cdfc5e3f4
📒 Files selected for processing (5)
.github/workflows/build-test.ymlREADME.mdtools/test_ci_build_provenance.shtools/test_docs_ci_drift.shtools/test_fuzz_seconds_validation.sh
Comment on lines
+60
to
+65
| if [ -f CONTRIBUTING.md ] && grep -qi 'fuzz.*-Werror\|-Werror.*fuzz' CONTRIBUTING.md; then | ||
| if ! grep -rq -- '-Werror' fuzz/ 2>/dev/null && ! grep -rlq -- '-Werror' .github/workflows/*.yml 2>/dev/null; then | ||
| echo "FAIL: CONTRIBUTING.md claims the fuzz harness builds with -Werror, but no fuzz build step passes it" | ||
| fail=1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bind the -Werror check to the fuzz build.
An unrelated workflow containing -Werror satisfies Line 61, so this regression test can pass while the fuzz harness no longer uses that flag. Restrict the workflow check to the fuzz build step/workflow.
🤖 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 `@tools/test_docs_ci_drift.sh` around lines 60 - 65, Update the -Werror
validation in the drift check around the grep conditions so workflow matches are
restricted to the fuzz build step or fuzz-specific workflow, rather than any
.github workflow containing -Werror. Preserve the existing direct fuzz/ source
check and failure message, ensuring unrelated workflow flags cannot satisfy the
test.
Comment on lines
+29
to
+37
| case "$RAW_FUZZ_SECONDS" in | ||
| '' | *[!0-9]*) | ||
| return 1 | ||
| ;; | ||
| esac | ||
| if [ "$RAW_FUZZ_SECONDS" -gt 14400 ]; then | ||
| return 1 | ||
| fi | ||
| printf 'FUZZ_SECS=%s\n' "$RAW_FUZZ_SECONDS" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
raw='999999999999999999999999999999999999'
if [ "$raw" -gt 14400 ]; then
echo rejected
else
echo accepted
fiRepository: myguard-labs/nginx-zstd-module
Length of output: 260
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '=== tools/test_fuzz_seconds_validation.sh ===\n'
wc -l tools/test_fuzz_seconds_validation.sh
sed -n '1,140p' tools/test_fuzz_seconds_validation.sh
printf '\n=== search FUZZ_SECS / fuzz seconds references ===\n'
rg -n "FUZZ_SECS|RAW_FUZZ_SECONDS|fuzz seconds|fuzz_seconds|14400" . -g '!**/.git/**' -g '!**/node_modules/**'
printf '\n=== tools/test_ci_build_provenance.sh ===\n'
if [ -f tools/test_ci_build_provenance.sh ]; then
wc -l tools/test_ci_build_provenance.sh
sed -n '1,220p' tools/test_ci_build_provenance.sh
fiRepository: myguard-labs/nginx-zstd-module
Length of output: 12071
Reject oversized digit strings before the -gt compare. A very long numeric input makes [ "$RAW_FUZZ_SECONDS" -gt 14400 ] error out, so validate can still emit FUZZ_SECS for an over-budget duration. Add a range-safe guard before the numeric comparison and a regression case for an oversized digit string.
🤖 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 `@tools/test_fuzz_seconds_validation.sh` around lines 29 - 37, Update the
validation logic around RAW_FUZZ_SECONDS in validate to reject digit strings
exceeding the supported range before invoking the [ numeric comparison,
preventing arithmetic errors from reaching the FUZZ_SECS output. Add a
regression case in the test script using an oversized all-digit input and verify
validation fails without emitting FUZZ_SECS.
Comment on lines
+86
to
+97
| # --- drift guard: fail loudly if the workflow's validation logic changes | ||
| # shape without this test being updated to match --- | ||
| if ! grep -q "'\*\[!0-9\]\*'" "$WORKFLOW" 2>/dev/null; then | ||
| if ! grep -q '\*\[!0-9\]\*' "$WORKFLOW"; then | ||
| echo "FAIL: ci-deep.yml's non-numeric guard pattern not found -- validate() above may have drifted from the live workflow" | ||
| fail=1 | ||
| fi | ||
| fi | ||
| if ! grep -q '14400' "$WORKFLOW"; then | ||
| echo "FAIL: ci-deep.yml no longer mentions the 14400s budget -- validate() above may have drifted from the live workflow" | ||
| fail=1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== repo files ==\n'
git ls-files tools/test_fuzz_seconds_validation.sh tools/test_ci_build_provenance.sh tools/ci-build.sh .github/workflows/ci-deep.yml
printf '\n== outlines ==\n'
for f in tools/test_fuzz_seconds_validation.sh tools/test_ci_build_provenance.sh tools/ci-build.sh .github/workflows/ci-deep.yml; do
[ -f "$f" ] && { echo "--- $f"; wc -l "$f"; sed -n '1,220p' "$f" | cat -n | sed -n '1,220p'; }
doneRepository: myguard-labs/nginx-zstd-module
Length of output: 32213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== exact references ==\n'
rg -n "grep -q|env|dispatch|validated|non-numeric|14400|guard|tar -xzf|guarded-mv|mv " tools/test_fuzz_seconds_validation.sh tools/test_ci_build_provenance.sh tools/ci-build.sh .github/workflows/ci-deep.yml
printf '\n== structural map for ci-build ==\n'
ast-grep outline tools/ci-build.sh --view expanded || trueRepository: myguard-labs/nginx-zstd-module
Length of output: 4678
Assert the workflow wiring, not just the copied guard.
tools/test_fuzz_seconds_validation.shcan still pass iffuzz_secondsis interpolated directly intorun:; add a check for theenv: RAW_FUZZ_SECONDShandoff.tools/test_ci_build_provenance.sh’s offline F2 path is still a local replay; if you want it to catch drift intools/ci-build.shitself, cover the real script with a fixture-based offline case.
📍 Affects 2 files
tools/test_fuzz_seconds_validation.sh#L86-L97(this comment)tools/test_ci_build_provenance.sh#L104-L110
🤖 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 `@tools/test_fuzz_seconds_validation.sh` around lines 86 - 97, The workflow
validation test must verify the env: RAW_FUZZ_SECONDS handoff rather than only
the copied guard and budget patterns; update
tools/test_fuzz_seconds_validation.sh at lines 86-97 to assert that wiring.
Strengthen the offline F2 coverage in tools/test_ci_build_provenance.sh at lines
104-110 by executing the real tools/ci-build.sh with a fixture-based offline
case instead of only replaying equivalent local logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #82 fixed all 7 audit findings (sha e289021) but only F5 shipped with an automated regression test. This adds coverage for F1/F2/F3/F7 so they can't silently regress; F4/F6 are documented as accepted gaps (see commit body / issues.md).
tools/test_fuzz_seconds_validation.sh— F1 dispatch-input injection guardtools/test_ci_build_provenance.sh— F2 clean-root mv-guard + F3 provenance rejection (wrong sha256, corrupted cache-hit, live clean-root angie build)tools/test_docs_ci_drift.sh— F7 docs-vs-CI structural drift check; caught and fixed a real gap (bump.yml undocumented in README)Wired into
build-test.yml'svalidationjob.Test plan
Summary by CodeRabbit
Bug Fixes
Documentation
Tests