feat: add tools/benchmarks list, --aggregation, and debug log-level to integration tests

[k-rister]

Summary

• Adds smoke + content-verification coverage for crucible tools list / crucible benchmarks list (crucible#655): bare invocation, --format table|json, --name, and a jq-based check that the JSON output is well-formed and non-empty (not just exit-code-0) — this last check exists because a broken aggregation could otherwise silently return {"tools": []} and still exit 0, which is exactly the class of bug found and fixed across two review rounds on crucible#655.
• Adds correctness coverage for crucible get metric's --aggregation sum|avg|max|min override, using mpstat/Busy-CPU (already queried elsewhere in this same test): confirms omitting --aggregation reproduces the metric's stored default-aggregation, that min <= avg <= max holds, and that sum != avg specifically — the last check exists because min<=avg<=max alone would pass even if --aggregation were silently ignored server-side and all four values came back identical.
• Adds a --ci-log-level option (default debug) applied to every crucible run --from-file-equivalent invocation, so a CI failure has enough detail (module/function/line-number annotated messages) to debug without reproducing it manually. Deliberately debug, not verbose-debug — the latter also cranks roadblock's own protocol tracing to its most verbose mode end-to-end, useful when actively chasing a roadblock-specific issue but too noisy to run unconditionally on every CI run. Only threaded through the "run-file" parameter mode (which runs unconditionally across every release); the "mv-params" legacy CLI mode is separately restricted to releases old enough that --log-level was never a thing there.
• Drops the now-deprecated --from-file from crucible run invocations (silently discarded as a no-op since crucible commit 7d11fa7, predating every release in the CI matrix).

All new/changed behavior is gated on CI_RELEASE consistent with this file's existing convention, verified via git merge-base --is-ancestor against each feature's actual introducing commit rather than assumption:

• crucible tools list/benchmarks list: upstream-only (crucible#655, not yet in any release branch including 2026.3)
• --aggregation: upstream-only (CommonDataModel commit 34539bb, 2026-07-30, postdates 2026.3)
• --log-level on crucible run: present in 2026.3 but not 2025.4/2026.1/2026.2

Test plan

• bash -n syntax-checked after every change
• jq extraction logic and float comparisons (min<=avg<=max, sum!=avg) tested against synthetic good/regression/missing-data cases before landing
• --log-level argument-order and word-splitting behavior verified empirically via CRUCIBLE_DRY_RUN=1 (old --from-file order vs. new positional order produce byte-for-byte identical constructed rickshaw-run.py commands)
• Live-verified crucible tools list/benchmarks list and the NOTICE/banner-stripping assumptions against the real CLI output
• Full CI run (this PR's own crucible-ci workflow will exercise all of the above for real)

[k-rister]

PR Review: crucible-ci#225 — feat: add tools/benchmarks list, --aggregation, and debug log-level to integration tests

Summary: Adds CI coverage for crucible tools/benchmarks list, crucible get metric --aggregation, applies --log-level debug to crucible run invocations, and drops the deprecated --from-file flag — all correctly version-gated by CI_RELEASE.
Changed files: 1 (.github/actions/integration-tests/run-ci-stage1)
Review dimensions: Correctness, API & Contracts, Build & Deploy, Documentation, Style, Completeness

Bugs

• [.github/actions/integration-tests/run-ci-stage1:372-410] --aggregation verification can print a false "verified" result using stale data from a previous period, once any earlier command in the run has failed. The five aggregation_*_value variables are plain globals, never reset or scoped per loop iteration, and the final consistency check (if [ -n "${aggregation_default_value}" ] && ...) is not guarded by [ ${RC_STATUS} == 0 ] — unlike the analogous tools_count/benchmarks_count check added later in this same PR, which correctly wraps its logic in if [ ${RC_STATUS} == 0 ].

  Concrete trigger: CI_SAMPLES defaults to 3 for the default userenv, so post_run_cmd's period loop runs 3+ times per call. Suppose period 1's aggregation block runs cleanly and sets all five values consistently (prints "verified"). Then period 2's preceding mpstat/Busy-CPU run_cmd (line 352) fails for any reason (transient SSH/OpenSearch hiccup, unrelated to --aggregation) — RC_STATUS latches nonzero. Reaching the aggregation case block for period 2, all five run_and_capture_cmd calls are silently skipped (their own internal guard: force != no -o RC_STATUS == 0), so aggregation_default_value/sum/avg/min/max retain period 1's leftover values. The final check still evaluates those stale, self-consistent numbers and prints "--aggregation override verified: default=... sum=... avg=... min=... max=..." for period 2 — a claim that is simply false, since none of period 2's queries ran.

  This doesn't flip the overall exit code (already nonzero from the earlier failure), but it actively misleads whoever is reading CI logs to debug a failure — directly undercutting this PR's own stated goal of making CI failures easier to diagnose. Fix: reset the five variables to empty before each attempt, and/or guard the final if/else on [ ${RC_STATUS} == 0 ] the same way the tools/benchmarks check does.

File Coverage

• .github/actions/integration-tests/run-ci-stage1 — 1 bug (stale-data false "verified" message in the --aggregation check)

Limitations

• Could not execute the script end-to-end against a live CI endpoint; correctness of the --aggregation/tools/benchmarks JSON-shape assumptions was verified by tracing CDM/crucible source (cdm.js, get-metric-data.js, list-subprojects.py) rather than running the commands live.
• Independently verified (via fresh bare clones, not relying on the PR description) that all four CI_RELEASE version-gate boundaries are accurate: crucible tools/benchmarks list (crucible#655, upstream-only, not in 2026.3), --aggregation (CommonDataModel 34539bb, postdates 2026.3), --log-level on crucible run (introduced 408698e, present in 2026.3, absent in 2025.4/2026.1/2026.2), and --from-file no-op since 7d11fa7 (predates every release in the matrix). No misgating found.
• Independently verified mpstat's Busy-CPU metric descriptor genuinely has default-aggregation: "sum" (sysstat-post-process.py:331), so the default == sum assumption underlying the new test is structurally sound, not a systematic failure waiting to happen.
• Ruled out a theoretical jq-parse fragility in the tools list/benchmarks list --format json check (no defensive sed filter for stdout preamble, unlike the aggregation check) as a non-issue in practice, since crucible-ci's installer always passes --name/--email, so check_id()'s conditional stdout noise never fires here.

Verdict

Approve with comments — the stale-data bug in the --aggregation check is real but only degrades log fidelity in an already-failing run; it doesn't cause false-positive CI passes or affect the exit code. Worth fixing before merge, but not blocking.

🤖 Generated with Claude Code

[k-rister]

Fixed in d2350b4.

The five aggregation_*_value variables are now reset to empty at the top of each period's attempt (no more stale carry-over from a prior period), and the final verify/error message is now gated on RC_STATUS == 0 — printing a plain "skipping" line instead when an earlier command in that period already failed. This matches the guard already used by the tools/benchmarks list check.

🤖 Generated with Claude Code

[atheurer]

Pull Request Review: crucible-ci PR #225

I have performed an in-depth review of PR #225 on the crucible-ci repository. The PR is a high-quality contribution that significantly improves the integration testing capabilities of the CI pipeline by adding correctness assertions, log levels for better debuggability, and new command coverage.

Below is a detailed analysis of the changes, design decisions, and logical verification.

---

1. Summary of Changes

The PR modifies a single file: .github/actions/integration-tests/run-ci-stage1. The changes can be grouped into four primary logical blocks:

1. Detailed Logging in CI (--ci-log-level / --log-level)

   • Introduces a new CI parameter --ci-log-level (defaulting to "debug").
   • Dynamically threads this as --log-level to crucible run invocations for releases 2026.3 and newer (including upstream).
   • Old releases (up to 2026.2) that do not support --log-level are correctly filtered out to prevent execution crashes.

2. Removal of Deprecated --from-file CLI Flag

   • Replaces the deprecated and ignored --from-file option with direct positional file passing:

     -run_cmd "crucible run --from-file ${CI_RUN_FILE}"
     +run_cmd "crucible run ${CI_RUN_FILE} ${CI_LOG_LEVEL_ARG}"

   • This prevents potential future breakage or deprecation errors while preserving backward compatibility.

3. Metric Aggregation Override Correctness Verification

   • Adds correctness assertions for crucible get metric --aggregation <sum|avg|min|max> on supported releases (newer than 2026.3).
   • Ensures that the default aggregation matches sum (the schema default for mpstat/Busy-CPU).
   • Mathematically verifies that min <= avg <= max holds true.
   • Asserts that sum != avg to verify that aggregation is actually occurring and not being silently ignored (which would cause all outputs to look identical and bypass the inequality assertions).
   • Resets state variables (aggregation_*_value) inside the period loop to avoid stale-value leakage and guards checks under RC_STATUS == 0.

4. Discovery Command Coverage (crucible tools list & crucible benchmarks list)

   • Adds integration test coverage for the new listing commands introduced in crucible #655.
   • Validates not only the zero exit codes but also parses the JSON output with jq to confirm they contain at least one tool/benchmark item and are well-formed.

---

2. Technical Evaluation & Logic Verification

A. Logging and Release Gating

The release gates are accurately mapped:

• --log-level: Only introduced in 2026.3.

  case "${CI_RELEASE}" in
      "2024.4"|"2025.1"|"2025.2"|"2025.3"|"2025.4"|"2026.1"|"2026.2")
          # Correctly skips older releases

• --aggregation override and listing commands: Only introduced after 2026.3.

  case "${CI_RELEASE}" in
      "2024.4"|"2025.1"|"2025.2"|"2025.3"|"2025.4"|"2026.1"|"2026.2"|"2026.3")
          # Correctly skips 2026.3 and older

This categorization is accurate and prevents command execution failure on older, pinned branches during backward-compatibility tests.

B. Defensive Scripting and Variable Isolation

In the final revision of the PR, the aggregation variables are properly reset on every period loop iteration:

aggregation_default_value=""
aggregation_sum_value=""
aggregation_avg_value=""
aggregation_max_value=""
aggregation_min_value=""

This is critical because if an earlier command fails in a subsequent period, run_and_capture_cmd will skip execution (leaving the variables unchanged), but the final check will now be correctly skipped because it is gated behind if [ ${RC_STATUS} == 0 ].

Furthermore, extracting the values with sed -n '/^{/,$p' is highly robust as it strips any leading header lines/roadblock notices before passing the clean JSON to jq.

C. Mathematical Comparison Logic via awk

Since bash does not support floating-point mathematics natively, using awk to perform float comparisons is excellent practice:

awk -v a="${aggregation_min_value}" -v b="${aggregation_avg_value}" 'BEGIN { exit !(a <= b) }'

This evaluates accurately and sets the correct bash exit status (0 for true, 1 for false).

The check sum != avg is particularly clever because it ensures the aggregation flag is not simply being ignored server-side. For mpstat/Busy-CPU, there are 7 subtypes, making the sum always significantly greater than the average.

---

3. Potential Enhancements & Minor Recommendations

While the PR is exceptionally well-engineered, here are a few minor recommendations to consider:

Recommendation 1: Handling potential empty/non-numeric values in awk comparisons

If one of the metrics fails to capture a numeric value, or if jq returns an empty string or null, the awk comparison might face syntax or type issues.
While the script guards against empty strings via [ -n "${variable}" ], a non-numeric string (e.g. "null" or "error") could still pass the -n check but fail in the awk comparison.

A small sanitizer or regex match to ensure the values are indeed numeric before passing to awk would make it bulletproof:

if [[ "${aggregation_default_value}" =~ ^[0-9.-]+$ ]] && ...

Recommendation 2: Quotation of CI_LOG_LEVEL_ARG in run_cmd Invocations

In run-ci-stage1, the crucible run command is invoked as:

run_cmd "crucible run ${CI_RUN_FILE} ${CI_LOG_LEVEL_ARG}"

Since run_cmd executes its first argument unquoted (e.g. ${cmd} on line 112 of common-code.sh), this unquoted expansion splits on whitespaces, which works perfectly here:

• If CI_LOG_LEVEL_ARG is "--log-level debug", it splits into two arguments.
• If CI_LOG_LEVEL_ARG is "", it splits into nothing (no empty argument passed to crucible).

This is correct and robust, but a developer reading the code might be tempted to quote ${CI_LOG_LEVEL_ARG} in the future (e.g., "${CI_LOG_LEVEL_ARG}"), which would cause crucible run to receive an empty-string argument and fail. Adding a quick inline comment explaining why it is unquoted could prevent future regressions.

---

4. Conclusion

This pull request is in excellent shape and ready to merge. The logical separation across the 5 commits is exemplary, and the defensive scripting strategies employed (such as variable resetting, release gating, and robust JSON stripping) make the CI pipeline exceptionally resilient.