Skip to content

feat: support --validate-only mode in rickshaw-run - #877

Merged
k-rister merged 6 commits into
masterfrom
feat-validate-only
Sep 1, 2026
Merged

feat: support --validate-only mode in rickshaw-run#877
k-rister merged 6 commits into
masterfrom
feat-validate-only

Conversation

@k-rister

Copy link
Copy Markdown
Contributor

Summary

Adds --validate-only support to rickshaw-run.py to enable deep static validation of run files, endpoint definitions, benchmark integration schemas, tool schemas, and utility configurations without requiring endpoint connectivity, container image sourcing, or engine deployment.

Also addresses an unhandled JSONDecodeError exception in blockbreaker.py and aligns agent instruction files with the AGENTS.md standard.

Changes

  • rickshaw-run.py:
    • Added --validate-only CLI parameter.
    • Skips validate_endpoints() (live node/host connectivity checks), image sourcing, engine deployment, and execution phases.
    • Validates run-file JSON schema, endpoint block schemas, benchmark integration definitions and parameters via multiplex, tool parameters, and utility parameters.
    • Suppresses routine orchestrator startup INFO logs during --validate-only at default (normal) log level.
    • Prints VALID and terminates cleanly with exit code 0 upon successful validation.
    • Preserves error logging and diagnostics on failure (sys.exit(1)).
  • util/blockbreaker.py:
    • Changed except JSONDecodeError as err: to except json.JSONDecodeError as err: in load_json_file() to prevent NameError tracebacks when decoding malformed JSON run files.
  • AGENTS.md / CLAUDE.md:
    • Moved canonical agent instructions to AGENTS.md and updated CLAUDE.md to @AGENTS.md following the crucible agent instruction guidelines.

Verification

  • Verified deep validation of valid run file outputs VALID with rc 0.
  • Verified validation of run file with malformed JSON outputs clean decode error without traceback and exits with rc 1.
  • Verified validation of run file with invalid schema exits with rc 1 and details error.
  • Verified --log-level=verbose still displays detailed progress logs when requested.

Add `--validate-only` flag to `rickshaw-run.py` to perform deep validation
of run files, benchmark parameters, tool parameters, and utility parameters
without attempting live endpoint connectivity, image sourcing, or engine
deployment. Output VALID on successful validation and suppress routine
startup INFO logs when validating at default log level.

Also catch `json.JSONDecodeError` in `blockbreaker.py` to prevent NameError
tracebacks when decoding malformed JSON run files, and document validation
mode in `CLAUDE.md`.
Move canonical agent instructions to AGENTS.md and update CLAUDE.md to
import AGENTS.md following the crucible agent instructions standard.
@k-rister k-rister self-assigned this Aug 30, 2026
@k-rister
k-rister requested a review from a team August 30, 2026 20:46
@project-crucible-tracking project-crucible-tracking Bot moved this to In Progress in Crucible Tracking Aug 30, 2026
@k-rister

Copy link
Copy Markdown
Contributor Author

PR Review: rickshaw#877 — feat: support --validate-only mode in rickshaw-run

Summary: Adds a --validate-only flag to rickshaw-run.py for deep static validation of run files (schema, benchmark/tool/utility params, multiplex expansion) without live endpoint connectivity or deployment, fixes an unhandled JSONDecodeError reference bug in blockbreaker.py, and migrates the repo's agent instructions to the AGENTS.md/CLAUDE.md convention.

Changed files: 4
Review dimensions: Correctness, API & Contracts, Build & Deploy, Documentation, Style, Completeness

Issues

  • [rickshaw-run.py:334] --validate-only=<value> ignores the value and always enables validate-only, even for --validate-only=falseprocess_cmdline() (line 334-336) and the duplicate scan in main() (line 2530) both only check startswith("validate-only=") and unconditionally set validate_only = True, discarding the actual value. Not reachable through crucible validate (which always passes the bare flag) or via a run-file's run-params (schema/run-file.json's run-params has additionalProperties: false and doesn't list validate-only), but the accept-any-value behavior is misleading if anyone invokes it with =value later.
  • [rickshaw-run.py] New --validate-only logic has no test coverage — None of tests/*.py or util/tests/*.py exercise process_cmdline()'s validate-only branch, the main() early-exit-with-VALID path, or the log-level suppression logic. A future refactor could silently break crucible validate's dependency on this exit/print contract without any test catching it.

File Coverage

  • rickshaw-run.py — 2 issues (see above)
  • util/blockbreaker.py — No issues. The except JSONDecodeErrorexcept json.JSONDecodeError fix is correct: the bare name was never imported/defined, so malformed JSON previously raised a masking NameError instead of the intended clean error message.
  • AGENTS.md — No issues. Correctly holds the full canonical content plus an accurate new "Validation Mode" section.
  • CLAUDE.md — No issues. Correctly reduced to the @AGENTS.md import per the documented convention.

Limitations

  • Did not execute rickshaw-run.py --validate-only end-to-end against live run files; correctness of the exit/print contract is verified by code reading, not live execution.
  • Skipping validate_endpoints() also skips assign_bench_ids()/validate_client_server_ids()/compute_engine_pairing(), so --validate-only won't catch client/server-engine-ratio mismatches or missing-required-server errors that only surface from live endpoint output. This is a scope limitation consistent with the PR's stated design rather than a bug, but worth noting when describing what "deep validation" covers.

Verdict

Approve with comments — no blocking issues; the two findings are a documentation/robustness nit on an unreachable edge case and a test-coverage gap the author can address at their discretion.

🤖 Generated with Claude Code

…rage

Support boolean argument parsing (--validate-only=true/false) in
rickshaw-run.py via parse_bool_arg(), and add unit tests covering
argument parsing, execution flow, early exit contract, and log level
suppression in tests/test_validate_only.py.
@k-rister

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Addressed both findings in commit f4d223f:

  1. --validate-only=<value> Argument Parsing: Added parse_bool_arg() helper in rickshaw-run.py to properly evaluate boolean argument values (true, false, 1, 0, yes, no, on, off). Both process_cmdline() and main() now parse --validate-only=<value> through this helper, correctly setting validate_only = False when --validate-only=false is passed and reporting an error/exiting with rc 1 on invalid values.
  2. Unit Test Coverage: Added tests/test_validate_only.py with 14 unit tests exercising:
    • parse_bool_arg() with boolean, truthy, falsy, and invalid inputs.
    • process_cmdline() CLI argument parsing (--validate-only, --validate-only=true, --validate-only=false, --validate-only=invalid, and default False).
    • main() execution flow (bypassing live endpoint validation, early exit with VALID on exit code 0).
    • Log level configuration (raising to WARNING in validation mode under normal log level, preserving INFO under verbose).

@atheurer

Copy link
Copy Markdown
Contributor

I found two actionable issues:

  1. [P1] --validate-only does not validate benchmark parameter files

The early exit at rickshaw-run.py:2601 occurs after load_bench_params(), but load_bench_params() never calls validate_schema() with bench-params.json. For example, an empty object or otherwise schema-invalid parameter file can result in VALID rather than a validation failure. Please validate each loaded benchmark parameter file against self.bench_params_schema_file before accepting it.

  1. [P1] --validate-only does not validate tool parameter schemas

Similarly, load_tool_params() loads the JSON but never validates it against self.tool_params_schema_file. Since validation exits at line 2601, malformed values such as "params": "invalid" can pass and print VALID (unless later code happens to access the malformed field). Add explicit tool-parameter schema validation before iterating over the entries.

When rickshaw-run was ported from Perl to Python 3 (de1b5af),
self.bench_params_schema_file and self.tool_params_schema_file were
initialized on RunState, but the validate_schema() calls were
inadvertently omitted from load_bench_params() and load_tool_params().

This pre-existing omission was discovered during the --validate-only
work, where early termination after parameter loading allowed malformed
parameter files (such as an empty object or non-array tool params) to
bypass downstream execution checks and falsely report validation success.

- Call validate_schema(param_sets, self.bench_params_schema_file) in
  load_bench_params().
- Call validate_schema(json_ref, self.tool_params_schema_file) in
  load_tool_params().
- Add unit test coverage in tests/test_validate_only.py for valid and
  invalid bench-params and tool-params structures.
@k-rister

Copy link
Copy Markdown
Contributor Author

Good catch! This was indeed a pre-existing omission dating back to the Perl-to-Python3 port (de1b5af), where self.bench_params_schema_file and self.tool_params_schema_file paths were initialized on RunState but the corresponding validate_schema() calls were omitted in load_bench_params() and load_tool_params().

Resolved in commit 7812c5c:

  1. Benchmark Parameter Schema Validation: Added validate_schema(param_sets, self.bench_params_schema_file) in load_bench_params().
  2. Tool Parameter Schema Validation: Added validate_schema(json_ref, self.tool_params_schema_file) in load_tool_params().
  3. Unit Tests: Added test cases in tests/test_validate_only.py verifying that invalid benchmark and tool parameter structures (empty objects, missing required fields, non-array types) exit with code 1 and valid structures succeed.

The rickshaw-run-tests workflow job did not install jsonschema into its
test virtualenv, causing schema validation tests in test_validate_only.py
to fail on the runner.

- Add jsonschema to pip install in .github/workflows/unittest.yaml.
- Add structural validation fallback in test_validate_only.py if jsonschema
  is unavailable.
@k-rister

Copy link
Copy Markdown
Contributor Author

Follow-up in commit 987b51d:

  • Added jsonschema to the rickshaw-run-tests workflow job in .github/workflows/unittest.yaml (aligning with the blockbreaker test job).
  • Added an ImportError structural check fallback in tests/test_validate_only.py to ensure unit tests run cleanly across all environments.

@atheurer

Copy link
Copy Markdown
Contributor

--validate-only currently skips validate_endpoints() entirely, but that is where endpoint-specific validation occurs. The endpoint validators check both the run-file schema and the type-specific endpoint schema (kube.json, remotehosts.json, etc.), so malformed endpoint options can result in VALID without being checked.\n\nCould validation mode retain the static checks while skipping live connectivity/readiness checks? For example, add a dedicated validate_endpoint_schemas() path (or a static_only mode) that validates schema/run-file.json and each endpoint block against its type-specific schema, but avoids host/cluster discovery and connectivity.

Add static schema validation for run-file and endpoint configurations via RunState.validate_endpoint_schemas(). This ensures that run-file structure and type-specific endpoint configurations (remotehosts.json, kube.json, etc.) are validated against their JSON schemas in --validate-only mode as well as during normal execution before live endpoint checks.
@k-rister

Copy link
Copy Markdown
Contributor Author

Added static run-file and endpoint schema validation in commit 654c3c6:

  1. Static Endpoint Schema Validation: Added RunState.validate_endpoint_schemas() in rickshaw-run.py, which validates schema/run-file.json and checks each declared endpoint block against its type-specific schema (schema/remotehosts.json, schema/kube.json, schema/osp.json, etc.) without performing host discovery, cluster queries, or network connectivity checks.
  2. Early Invocation: Called validate_endpoint_schemas() in main() so that static schema validation runs during both --validate-only mode and normal runs prior to live endpoint deployment.
  3. Unit Test Coverage: Added TestEndpointSchemaValidation test suite in tests/test_validate_only.py covering valid and invalid run-file and endpoint definitions across remotehosts and kube endpoint types.

@k-rister
k-rister merged commit bdbc217 into master Sep 1, 2026
419 of 421 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Crucible Tracking Sep 1, 2026
@k-rister
k-rister deleted the feat-validate-only branch September 1, 2026 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants