From ab437a15dfbbfce43dfbfa38714612bb5ea08c39 Mon Sep 17 00:00:00 2001 From: Oleksandr Bezdieniezhnykh Date: Tue, 12 May 2026 16:32:36 +0300 Subject: [PATCH] [AZ-504] Fix grep | wc -l pipefail crash in PT-08 batch counting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scripts/run-performance-tests.sh:416-417 used `grep -o ... | wc -l` to count `"status":"accepted"` and `"status":"rejected"` markers in the PT-08 batch response. On the happy path (rejected=0) grep -o exits 1, and under `set -o pipefail` + `set -e` (line 16) the pipeline killed the script before reaching any of PT-08's reporting code — reproducing twice in the cycle-3 perf-harness leftover (replay #2 + #3 post-AZ-500). Fix: neutralise grep's no-match exit locally with `|| true` on the grep stage of each pipeline. `grep -o | wc -l` is kept (not swapped for `grep -c`) because the PT-08 response is compact JSON — all items live on one line, so `grep -c` would always return 1 and lose occurrence-count semantics. An 8-line comment explains why grep cannot fail for I/O at this code path (file is curl-written, HTTP 200 gated). AC-1 + AC-2 verified in-place against a standalone harness under `set -e -o pipefail` (compact-JSON, mixed-status, edge-empty cases). AC-3 + AC-4 are Step 15 (Performance Test) obligations by spec design — the leftover deletion (AC-4) is "in the same commit" as the green full perf run. Batch report: _docs/03_implementation/batch_01_cycle5_report.md. Code review: _docs/03_implementation/reviews/batch_01_cycle5_review.md — PASS, no findings. Co-authored-by: Cursor --- .../AZ-504_perf_script_grep_pipefail_fix.md | 0 .../batch_01_cycle5_report.md | 60 +++++++++++++++++++ .../reviews/batch_01_cycle5_review.md | 31 ++++++++++ scripts/run-performance-tests.sh | 12 +++- 4 files changed, 101 insertions(+), 2 deletions(-) rename _docs/02_tasks/{todo => done}/AZ-504_perf_script_grep_pipefail_fix.md (100%) create mode 100644 _docs/03_implementation/batch_01_cycle5_report.md create mode 100644 _docs/03_implementation/reviews/batch_01_cycle5_review.md diff --git a/_docs/02_tasks/todo/AZ-504_perf_script_grep_pipefail_fix.md b/_docs/02_tasks/done/AZ-504_perf_script_grep_pipefail_fix.md similarity index 100% rename from _docs/02_tasks/todo/AZ-504_perf_script_grep_pipefail_fix.md rename to _docs/02_tasks/done/AZ-504_perf_script_grep_pipefail_fix.md diff --git a/_docs/03_implementation/batch_01_cycle5_report.md b/_docs/03_implementation/batch_01_cycle5_report.md new file mode 100644 index 0000000..ea99eff --- /dev/null +++ b/_docs/03_implementation/batch_01_cycle5_report.md @@ -0,0 +1,60 @@ +# Batch Report + +**Batch**: 01 (cycle 5) +**Tasks**: AZ-504 — Perf script: fix grep | wc -l pipefail crash in PT-08 +**Date**: 2026-05-12 + +## Task Results + +| Task | Status | Files Modified | Tests | AC Coverage | Issues | +|------|--------|---------------|-------|-------------|--------| +| AZ-504_perf_script_grep_pipefail_fix | Done (AC-1/AC-2 in-place verified; AC-3/AC-4 deferred to Step 15) | 1 file | pass (in-place harness, 4 cases under `set -e -o pipefail`) | 2/4 covered now; 2/4 deferred to Step 15 by spec design | None | + +## Changes + +`scripts/run-performance-tests.sh:416-417` — replaced + +```bash +accepted=$(grep -o '"status":"accepted"' "$PERF_TMP_DIR/pt08_resp.json" | wc -l | tr -d ' ') +rejected=$(grep -o '"status":"rejected"' "$PERF_TMP_DIR/pt08_resp.json" | wc -l | tr -d ' ') +``` + +with + +```bash +# AZ-504: grep exits 1 on zero matches. Under `set -o pipefail` (line 16) +# that kills the assignment and crashes the script on the happy path +# (rejected=0). Neutralise the no-match case locally with `|| true` so +# the pipeline still produces a count. The response is compact JSON +# (one line, all items) so `grep -o | wc -l` is required to count +# occurrences — `grep -c` would only count matching lines (=1). The +# file is guaranteed-readable here (curl wrote it earlier in this +# iteration on the HTTP 200 branch), so grep cannot fail for I/O. +accepted=$({ grep -o '"status":"accepted"' "$PERF_TMP_DIR/pt08_resp.json" || true; } | wc -l | tr -d ' ') +rejected=$({ grep -o '"status":"rejected"' "$PERF_TMP_DIR/pt08_resp.json" || true; } | wc -l | tr -d ' ') +``` + +**Why `|| true` on grep instead of `grep -c`**: PT-08 response is compact JSON written by ASP.NET Core's default serializer — all items live on a single line of the response body. `grep -c` counts MATCHING LINES, so it would return 1 regardless of whether there are 1 or 10 accepted items (Risk 1 of the task spec). `grep -o` + `wc -l` preserves occurrence semantics. The `|| true` is the minimum local neutralisation of pipefail and is justified by the `coderule.mdc` exception "If an error is truly safe to ignore, log it or comment why" — the 8-line comment explains why grep cannot fail for I/O at this code path (file is curl-written, HTTP-200-gated). + +**Shellcheck pass over the file (Risk 2 of task spec)**: surveyed all `grep ... | wc -l` and `grep -[oc]` sites. Only two `grep -o ... | wc -l` sites exist in the script — both at lines 416-417 (the ones fixed). The third `grep -o` site (line 141, region status polling) is already protected by `head -1 || true` and is not vulnerable. No other defensive work required. + +## AC Test Coverage + +| AC | Status | Where verified | +|----|--------|----------------| +| AC-1 — PT-08 completes on zero-rejected response | **Covered** | Standalone harness reproduces the exact pipeline under `set -e -o pipefail` with `accepted=2 rejected=0`; pipeline returns count `0` without script exit. | +| AC-2 — PT-08 completes on zero-accepted response (defensive) | **Covered** | Same harness with `accepted=0 rejected=2`; pipeline returns count `0` for accepted without script exit. | +| AC-3 — PT-08 summary line prints in full run | **Deferred to Step 15** | Requires `docker-compose up -d --build` + full default-parameter `./scripts/run-performance-tests.sh` (PERF_REPEAT_COUNT=20 PERF_UAV_BATCH_SIZE=10). This is exactly the Step 15 (Performance Test) gate for cycle 5. The spec is staged: AC-4 GIVEN clause depends on AC-3 + "the full perf run is green", so the natural verification environment is Step 15. | +| AC-4 — Leftover deletion on green full run | **Deferred to Step 15** | By spec design — AC-4 says the leftover file is deleted "in the same commit" as the green full perf run. That commit IS the Step 15 deliverable. Confirmed in the perf-cycle3 leftover's "Replay attempt #4" entry. | + +**Spec-Gap?** No. AC-3 + AC-4 are not gaps for Step 10 — they are explicit Step 15 deliverables baked into the spec itself (AC-4 GIVEN clause). Step 10 (this batch) delivers the script fix that makes Step 15 possible. + +## Test Strategy Note + +This project has no shell-script unit test infrastructure (no BATS, no `scripts/test_*.sh`). The established pattern is "the script is the test" — `scripts/run-tests.sh` and `scripts/run-performance-tests.sh` ARE the verification surface. Adding BATS for a 1-SP single-line fix would be infra creep explicitly disallowed by `coderule.mdc` ("Avoid boilerplate and unnecessary indirection ... follow established project patterns"). The standalone harness recorded above is repeatable on demand and produces evidence equivalent to a BATS smoke test. + +## Code Review Verdict: PASS +## Auto-Fix Attempts: 0 +## Stuck Agents: None + +## Next Batch: AZ-503 — Tile identity → UUIDv5 + integer UPSERT + bulk-list endpoint (3 SP) diff --git a/_docs/03_implementation/reviews/batch_01_cycle5_review.md b/_docs/03_implementation/reviews/batch_01_cycle5_review.md new file mode 100644 index 0000000..211f037 --- /dev/null +++ b/_docs/03_implementation/reviews/batch_01_cycle5_review.md @@ -0,0 +1,31 @@ +# Code Review Report + +**Batch**: 01 (cycle 5) — AZ-504 (1 SP) +**Date**: 2026-05-12 +**Verdict**: PASS + +## Phase Summary + +| Phase | Result | +|-------|--------| +| 1. Context Loading | OK — task spec read; intent = tolerate grep exit 1 on no-match while preserving occurrence-count semantics; do NOT change PT-08 threshold or production code. | +| 2. Spec Compliance | OK — AC-1 + AC-2 verified in standalone harness under `set -e -o pipefail`. AC-3 + AC-4 staged by spec design (AC-4 GIVEN clause couples them to the Step 15 full perf run). No Spec-Gap. | +| 3. Code Quality | OK — `\|\| true` is locally scoped, accompanied by an 8-line comment explaining why grep cannot fail for I/O at this code path; coderule.mdc allows this with comment. Mirror-image accepted/rejected lines are intentional, not duplication worth extracting. | +| 4. Security Quick-Scan | OK — no SQL, no command injection, no secrets, no input validation touch. | +| 5. Performance Scan | OK — same `grep -o \| wc -l` complexity as before; `\|\| true` adds zero overhead on the happy path. | +| 6. Cross-Task Consistency | N/A — single-task batch. | +| 7. Architecture Compliance | N/A — shell script, not in any C# component; no layer / Public API / cycle / duplicate-symbol concerns. | + +## Findings + +None. + +## Notes + +- Risk 1 (compact JSON vs `grep -c`) was verified empirically against `SatelliteProvider.Common/DTO/UavTileBatchUploadResponse.cs` — the ASP.NET Core default serializer emits the full `Items` list on a single line. `grep -c` would have collapsed all occurrences to a line count of 1. The chosen fix (`grep -o ... \|\| true \| wc -l`) preserves occurrence semantics. The 8-line in-script comment captures this so a future maintainer doesn't re-introduce `grep -c`. +- Risk 2 (other vulnerable `grep ... \| wc -l` sites) — Grep pass over the whole script found only the two sites named in the spec (416, 417). The third `grep -o` site (line 141, region status polling) is protected by `head -1 \|\| true`. No other defensive work required. +- Test infra note — no BATS/shell-test framework in this project. Adding one for a 1-SP fix is infra creep. The standalone harness in the batch report is the established equivalent verification. + +## Verdict + +**PASS** — no findings of any severity. Proceed to commit. AC-3 + AC-4 are Step 15 obligations, not Step 10 gaps. diff --git a/scripts/run-performance-tests.sh b/scripts/run-performance-tests.sh index 4eb04ed..dceaf4f 100755 --- a/scripts/run-performance-tests.sh +++ b/scripts/run-performance-tests.sh @@ -413,8 +413,16 @@ else continue fi - accepted=$(grep -o '"status":"accepted"' "$PERF_TMP_DIR/pt08_resp.json" | wc -l | tr -d ' ') - rejected=$(grep -o '"status":"rejected"' "$PERF_TMP_DIR/pt08_resp.json" | wc -l | tr -d ' ') + # AZ-504: grep exits 1 on zero matches. Under `set -o pipefail` (line 16) + # that kills the assignment and crashes the script on the happy path + # (rejected=0). Neutralise the no-match case locally with `|| true` so + # the pipeline still produces a count. The response is compact JSON + # (one line, all items) so `grep -o | wc -l` is required to count + # occurrences — `grep -c` would only count matching lines (=1). The + # file is guaranteed-readable here (curl wrote it earlier in this + # iteration on the HTTP 200 branch), so grep cannot fail for I/O. + accepted=$({ grep -o '"status":"accepted"' "$PERF_TMP_DIR/pt08_resp.json" || true; } | wc -l | tr -d ' ') + rejected=$({ grep -o '"status":"rejected"' "$PERF_TMP_DIR/pt08_resp.json" || true; } | wc -l | tr -d ' ') PT08_ACCEPTED=$((PT08_ACCEPTED + accepted)) PT08_REJECTED=$((PT08_REJECTED + rejected))