Establish performance baselines and regression detection

[dheerajodha]

What:

Store benchmark baselines in the repo and compare CI results against them. Fail the benchmark check when regressions exceed configurable thresholds.

Why:

EC-1818 added a report-only stress benchmark to CI. This follow-up closes the loop by detecting regressions automatically, without baselines and thresholds, the benchmark runs but nobody notices when performance degrades.

Tickets:

EC-1819

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: e0d18c69-64fe-40e3-b7ba-e402f629e072

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:06 PM UTC · Completed 1:25 PM UTC
Commit: 87c4a29 · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
acceptance	54.41% <ø> (-0.01%)	⬇️
generative	16.35% <ø> (-0.01%)	⬇️
integration	27.56% <ø> (-0.01%)	⬇️
unit	72.14% <ø> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fullsend-ai-review]

Review

Findings

Medium

• [data-inconsistency] Makefile:214 — The generate-baseline target records baseline metadata using shell defaults ${EC_STRESS_WORKERS:-10} but the Go code uses defaultWorkers = 35. When running make generate-baseline without setting EC_STRESS_WORKERS, the benchmark runs with 35 workers but baseline.json records "workers": 10. The CI workflow sets EC_STRESS_WORKERS=10 so CI is consistent, but local generation produces misleading metadata.
  Remediation: Change shell defaults to match Go defaults (${EC_STRESS_WORKERS:-35}) or have the Go benchmark emit its actual configuration.

• [logic-error] Makefile:189 — Pre-existing pattern rules benchmark_% and benchmark/%/data.tar.gz have each recipe line in a separate subshell (due to @ prefix on each line). The @cd benchmark/$* on one line has no effect on @go run . or @./prepare_data.sh on the next line — these execute in the repo root. This PR documents make benchmark_stress as a recommended entry point, but that target is currently broken.
  Remediation: Combine recipe lines with && continuation: @cd benchmark/$* && go run .

• [protected-path] .claude/skills/benchmark/SKILL.md, .github/workflows/benchmark.yaml, AGENTS.md — This PR modifies protected governance/infrastructure files. The PR provides sufficient context (Jira EC-1819, clear rationale for adding regression detection). Human approval is always required for protected-path changes, regardless of context.

Low

• [data-inconsistency] benchmark/stress/baseline.json:5 — The committed baseline records "workers": 10 which differs from the Go source default of defaultWorkers = 35. Internally consistent with the CI workflow and Makefile shell default, but may cause confusion when comparing against local runs.

• [missing-validation] benchmark/stress/compare.sh — The comparison script does not verify that the current benchmark run used the same worker/component counts as the baseline. If EC_STRESS_COMPONENTS or EC_STRESS_WORKERS differ between baseline generation and the current run, the comparison is misleading. The baseline JSON contains components and workers fields but compare.sh never checks them.

• [error-handling] .github/workflows/benchmark.yaml:87 — The "Compare against baseline" step's failures are not differentiated in the job summary. A parse error in compare.sh and a genuine regression threshold breach produce the same generic regression warning.

• [overly-broad-cache-restore] .github/workflows/benchmark.yaml:39 — actions/cache/restore uses path: '**' with a static key: main. While GitHub Actions cache scoping prevents cross-repo poisoning and the practical risk is low, narrowing the cache path to specific directories (Go module/build cache) would be better hygiene.

• [GHA-expression-injection] .github/workflows/benchmark.yaml:85 — ${{ steps.bench.outcome }} and ${{ steps.compare.outcome }} are interpolated directly in run: blocks. The values are constrained to fixed strings and are not exploitable, but passing them via env: variables would be safer if the pattern is extended later.

• [scope-creep] benchmark/stress/stress.go:168 — The --allow-past-effective-time flag addition is likely necessary (the benchmark uses --effective-time 2024-12-10T00:00:00Z, a hardcoded past date), but the PR body does not explain why this change is needed. A brief note would help reviewers.

• [action-pinning-consistency] .github/workflows/benchmark.yaml:46 — harden-runner is pinned to v2.20.1 while most other workflows use v2.20.0 (matches release.yaml). Consider aligning all workflows to the same version.

• [architectural-fit] .github/workflows/benchmark.yaml:1 — The workflow triggers on all pull_request events to main without path filtering. Docs-only PRs will trigger a 15-minute benchmark job unnecessarily. A paths filter would reduce CI resource waste.

Previous run

Review

Findings

High

• [protected-path] .github/workflows/benchmark.yaml — This PR adds a file under the protected .github/ path. The PR has no linked GitHub issue (only a Jira ticket EC-1819 reference in the body). Human approval is required for all changes to governance and infrastructure files. Affected protected file: .github/workflows/benchmark.yaml.

Medium

• [logic-error] Makefile:214 — The generate-baseline target records the worker count using $${EC_STRESS_WORKERS:-10} (default 10), but stress.go defines defaultWorkers = 35 when EC_STRESS_WORKERS is unset. Running make generate-baseline without setting the env var produces a baseline.json with "workers": 10 while the actual benchmark ran with 35 workers. The baseline metadata would misrepresent the conditions under which it was generated.
  Remediation: Change the Makefile default to $${EC_STRESS_WORKERS:-35} to match the Go default, or export the env var in the recipe before invoking go run . so both the Go code and the Python metadata writer use the same value.

• [broken-dependency] Makefile:186 — The pre-existing pattern rule benchmark/%/data.tar.gz: uses separate recipe lines (@cd benchmark/$* then @./prepare_data.sh), each running in its own shell. The cd has no effect, so prepare_data.sh runs from the repo root and writes data.tar.gz to the wrong location. The new generate-baseline target depends on this broken rule and will not work correctly as written.
  Remediation: Fix the pattern rule to use a single shell command: @cd benchmark/$* && ./prepare_data.sh.

• [missing-documentation-update] .claude/skills/benchmark/SKILL.md — The benchmark skill file does not mention the new baseline/regression detection workflow, including make generate-baseline, compare.sh, baseline.json, or thresholds.json. The skill's trigger keywords also lack 'baseline', 'regression', and 'generate-baseline', so the skill won't activate when users ask about those topics.
  Remediation: Add a step documenting the baseline workflow and add relevant trigger keywords.

Low

• [edge-case] .github/workflows/benchmark.yaml:113 — In the 'Write job summary' step, if the Python regex fails to match, the process substitution silently fails and variables receive empty values. Since process substitution exit status is not captured by set -e in bash, the script continues with zero values and writes a misleading summary (0.0s, 0 MB) rather than reporting an error. The comparison step (compare.sh) handles this case correctly.

• [logic-error] benchmark/stress/compare.sh:62 — The zero-value guard only checks baseline values but not current values. If a current metric were somehow zero (unlikely given the Python parse validation, but possible), the percentage change would be -100% and pass the threshold check.

• [scope-alignment] benchmark/stress/stress.go:168 — The addition of --allow-past-effective-time is a behavioral fix to the benchmark itself (needed because the hardcoded --effective-time 2024-12-10T00:00:00Z is now far enough in the past that the CLI likely rejects it without this flag), not a baseline/regression-detection change. The PR body does not document this fix.

• [architectural-coherence] Makefile:200 — The generate-baseline target embeds a multi-line Python3 script inline in the Make recipe. Existing benchmark infrastructure follows a pattern of separate shell scripts (e.g., prepare_data.sh, compare.sh). Consider extracting to a standalone script for consistency and testability.

• [architectural-coherence] .github/workflows/benchmark.yaml:101 — The job summary step embeds ~65 lines of Python3 and shell parsing logic inline in the workflow YAML, duplicating parsing patterns from compare.sh. Extracting to a shared script would reduce this DRY violation.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (2)

Review

Findings

High

• [command-injection] benchmark/stress/compare.sh:49 — The shell variable $line is interpolated into a Python triple-quoted string literal (line = '''${line}'''). If the benchmark output contains ''', the Python string is terminated and arbitrary code executes. In CI, the benchmark binary is built from PR code, so a malicious PR can craft output to achieve code execution.
  Remediation: Pass the benchmark line to Python via an environment variable (os.environ['BENCH_LINE']) or stdin instead of shell interpolation into a string literal.

• [command-injection] .github/workflows/benchmark.yaml:123 — Same triple-quote injection in the "Write job summary" step (line = '''$line'''). The benchmark binary is built from PR code, enabling the same attack vector as above in the CI runner context.
  Remediation: Use env: BENCH_LINE: $line at the step level and reference os.environ['BENCH_LINE'] in Python.

• [protected-path] .github/workflows/benchmark.yaml — This PR adds a new file under .github/, which is a protected path requiring human approval. The PR references external tracker EC-1819 but has no linked GitHub issue, providing insufficient authorization context for governance file changes.

Medium

• [logic-error] .github/workflows/benchmark.yaml:88 — The "Compare against baseline" step runs unconditionally even when the benchmark step fails (which has continue-on-error: true). When the benchmark crashes, compare.sh exits 1 with "No BenchmarkStress results found" — conflating infrastructure failures with performance regressions.
  Remediation: Add if: steps.bench.outcome == 'success' to the Compare step.

Low

• [command-injection] benchmark/stress/compare.sh:57 — File paths ${BASELINE} and ${THRESHOLDS} are interpolated into Python open() calls. If the script directory path contained a single quote, it would break the Python string. Practically unlikely since paths are derived from BASH_SOURCE[0], but violates defense-in-depth.

• [logic-error] Makefile:183 — The generate_baseline target depends on the pre-existing benchmark/%/data.tar.gz rule, which has a broken multi-line recipe (each @ line runs in a separate shell, losing the cd). make generate_baseline will fail if data.tar.gz has not been manually prepared.

• [naming-convention] Makefile:183 — The generate_baseline target uses underscores. While the benchmark section locally follows this convention (benchmark_data, benchmark_%), the broader Makefile prefers hyphens (dist-container, tools-ci, lint-fix).

• [scope-alignment] benchmark/stress/stress.go:168 — The --allow-past-effective-time flag addition fixes a pre-existing issue required for the benchmark to function with the hardcoded past date, but is not mentioned in the PR description.

• [edge-case] benchmark/stress/compare.sh:61 — Division by zero in awk if baseline values are 0 (practically impossible for real benchmarks but unguarded).

• [action-version-consistency] .github/workflows/benchmark.yaml:48 — The harden-runner action pins to v2.20.1 (b09bb98e...), matching release.yaml but diverging from the v2.20.0 (bf7454d0...) used by other workflows.

• [stale-documentation] .claude/skills/benchmark/SKILL.md — The benchmark skill document does not mention the new baseline generation, regression detection, compare.sh, or the CI workflow introduced by this PR.

---

Labels: PR adds a GitHub Actions workflow with command injection vulnerabilities in shell/Python interpolation

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (3)

Review

Findings

High

• [protected-path] .github/workflows/benchmark.yaml — This PR adds a new file under the .github/ protected path. The PR has no linked GitHub issue establishing authorization for governance/infrastructure changes. Human approval is always required for protected-path changes.
  Remediation: Link a GitHub issue that authorizes adding a new CI workflow, or obtain explicit human reviewer approval for the .github/ change.

Medium

• [edge-case] benchmark/stress/compare.sh:48 — When grep -oP does not match a metric in the benchmark output (e.g., if the benchmark driver changes its output format), current_ns or current_rss will be an empty string. Awk treats empty strings as 0, causing silent -100% change computation and a false PASS despite having no valid measurement.
  Remediation: Validate extracted values are non-empty before comparison: if [[ -z "$current_ns" || -z "$current_rss" ]]; then echo 'Failed to parse benchmark metrics'; exit 1; fi

• [logic-error] Makefile:200 — The benchmark_baseline target extracts ns_op and peak_rss via grep but does not validate that the pipeline produced non-empty results. If the metric pattern does not match, printf will emit invalid JSON (e.g., "execution_time_ns": ,), silently overwriting the baseline with malformed data.
  Remediation: Add validation: [[ -n "$$ns_op" && -n "$$peak_rss" ]] || { echo 'Failed to parse benchmark output'; exit 1; }

• [architectural-coherence] .github/workflows/benchmark.yaml:60 — The workflow uses actions/cache/restore with key: main and path: '**', a very broad cache restore. No existing workflow uses this pattern, and there is no corresponding actions/cache/save step, so this cache will never be populated by this workflow.
  Remediation: Add a corresponding cache/save step, clarify which workflow populates this cache, or remove the cache/restore step.

• [intent-alignment] .github/workflows/benchmark.yaml:37 — The workflow uses continue-on-error: true at the job level, meaning the benchmark job can never cause a PR check to fail. The compare step also has continue-on-error: true. The PR body states "Fail the benchmark check when regressions exceed configurable thresholds," but the implementation cannot fail any check — regression detection is advisory-only.
  Remediation: Remove job-level continue-on-error: true to enable regression blocking, or update the PR body to reflect advisory-only intent.

• [missing-doc] benchmark/README.md:14 — The PR adds baseline comparison infrastructure (baseline.json, compare.sh, thresholds.json, benchmark_baseline target, CI workflow) but benchmark/README.md does not document any of it.
  Remediation: Add a section covering the baseline workflow, threshold configuration, and comparison script.

Low

• [logic-consistency] Makefile:208 — execution_time_ns in baseline.json is sourced from the ns/op metric. ns/op is nanoseconds per single iteration, while execution_time_ns implies total wall-clock time. Consistent today (driver.Parallel runs with n=1) but could mislead if configuration changes.

• [robustness] .github/workflows/benchmark.yaml:100 — grep -oP (PCRE mode) is a GNU extension. While ubuntu-latest includes PCRE support, this could break on custom runners or macOS (for the Makefile target).

• [naming-convention] Makefile:200 — benchmark_baseline collides with the existing benchmark_% pattern target. Make resolves explicit targets over pattern rules so there is no functional bug, but the name suggests benchmarking a "baseline" directory rather than generating baseline data.

• [shell-conventions] benchmark/stress/compare.sh:56 — Inconsistent variable expansion style. Existing scripts (prepare_data.sh, push_data.sh) consistently use ${VAR} brace-quoted expansion; compare.sh mixes $var and ${VAR} forms.

• [workflow-conventions] .github/workflows/benchmark.yaml:20 — Existing workflows trigger on both pull_request and push for main and release branches. This workflow only triggers on pull_request for main.

• [Makefile-conventions] Makefile:200 — Minor convention difference in @ prefix usage in the benchmark_baseline target.

• [missing-authorization] — The PR references external ticket EC-1819 but has no linked GitHub issue in conforma/cli. Non-trivial changes should ideally have a GitHub issue for tracking.

• [scope-creep] benchmark/stress/stress.go:168 — The --allow-past-effective-time flag is a prerequisite fix to the existing benchmark runner, not part of baseline/regression detection. Worth documenting in the PR description.

• [missing-doc] .claude/skills/benchmark/SKILL.md:54 — The benchmark skill does not mention the new baseline comparison workflow.

• [missing-doc] AGENTS.md:9 — AGENTS.md lists key Makefile targets but omits benchmark targets. Pre-existing gap widened by this PR.

---

Labels: PR adds CI benchmark workflow and benchmark infrastructure

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (4)

Review

Findings

Medium

• [logic-error] Makefile:209 — The benchmark_baseline target defaults EC_STRESS_WORKERS to 35 (${EC_STRESS_WORKERS:-35}), while the CI workflow hardcodes EC_STRESS_WORKERS: "10" and the checked-in baseline.json records "workers": 10. Running make benchmark_baseline without explicitly setting EC_STRESS_WORKERS=10 generates a baseline with 35 workers that CI will compare against using 10 workers, producing an unfair comparison that could mask regressions or trigger false positives.
  Remediation: Change ${EC_STRESS_WORKERS:-35} to ${EC_STRESS_WORKERS:-10} in the Makefile to match CI.

• [edge-case] benchmark/stress/compare.sh:55 — current_ns and current_rss are extracted via grep -oP with no validation that they are non-empty. If the benchmark output format changes and metrics are missing, grep produces empty strings and awk treats them as 0, causing a misleading −100% regression that fails CI. The same gap exists in the workflow's "Write job summary" step where $peak_rss and $ns_op are used without fallbacks in the rss_change/time_change calculations.
  Remediation: Add guards after extraction: if [[ -z "$current_ns" || -z "$current_rss" ]]; then echo "ERROR: Could not parse benchmark metrics."; exit 1; fi

• [protected-path] .github/workflows/benchmark.yaml — This PR adds a file under .github/, which is a protected path requiring human approval. The PR body provides rationale and references Jira ticket EC-1819. Human review of this CI workflow change is required regardless of automated review outcome.

Low

• [pattern-inconsistency] benchmark/stress/compare.sh:56 — compare.sh uses python3 for JSON parsing in 4 places, and the workflow uses it in 2 more. The rest of the codebase uses jq. Consider using jq for consistency, e.g.: baseline_ns=$(jq -r '.execution_time_ns' "${BASELINE}")

• [error-handling-idiom] Makefile:203 — The benchmark_baseline target suppresses stderr (2>/dev/null). If the benchmark panics or encounters build errors, no diagnostics are shown.

• [pattern-inconsistency] .github/workflows/benchmark.yaml:21 — Existing workflows (checks-codecov, lint, codeql) trigger on both pull_request and push. This workflow omits push, so merges to main won't produce benchmark runs. If intentional, a comment explaining the rationale would help.

• [edge-case] benchmark/stress/compare.sh:79 — The threshold comparison (change > thresh) only detects positive regressions. A dramatic improvement (e.g., −90%) passes silently, which could indicate a broken benchmark rather than genuine improvement.

• [code-organization] Makefile:200 — The benchmark_baseline target name matches the existing benchmark_% pattern rule. GNU Make prefers the explicit rule, but the overlap may confuse readers. (Note: benchmark_data and benchmark_simple follow the same pattern, so this is consistent with existing convention.)

• [missing-doc] benchmark/README.md — The benchmark README doesn't mention baseline generation, make benchmark_baseline, compare.sh, or threshold configuration.

• [missing-doc] AGENTS.md — AGENTS.md documents build/test commands but doesn't list benchmark make targets (pre-existing gap widened by this PR).

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:59 AM UTC · Completed 8:17 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:10 PM UTC · Completed 12:28 PM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:54 PM UTC · Completed 1:09 PM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:24 PM UTC · Completed 1:42 PM UTC

Commit: 87c4a29 · View workflow run →

Superseded by updated review