Address pesap review feedback on #271#272
Merged
daniel-thom merged 4 commits intomainfrom Apr 21, 2026
Merged
Conversation
- exec_cmd: skip `detect_spec_file_in_trailing` when `--` was used so `torc exec -- cat workflow.yaml` is run as a shell command instead of being flagged as a `torc run` mistake. Added a regression test. - cli: switch `--sample-interval-seconds` from `u32` (range `1..`) to `i32` (range `1..`) so the value can't wrap negative when assigned to `ResourceMonitorConfig::sample_interval_seconds`. Dropped the `as i32` cast in exec_cmd accordingly. - docs: fix `torc -s tui` example (the top-level `-s` is ignored for `tui`; the working invocation is `torc tui --standalone`). Move the `torc exec` how-to out of the first slot in SUMMARY / how-to index / introduction Key Features since it's an alternative entry point, not the main workflow.
Adds a new HPC how-to covering the sbatch patterns enabled by the ephemeral torc-server in #271: - `torc -s run workflow.yaml` for single-node spec-driven jobs - `torc -s exec -C commands.txt` for single-node ad-hoc command queues - How to inspect the persisted database from the login node afterwards Also documents the multi-node alternative (persistent torc-server bound to a routable hostname + `srun`) and explains the standalone vs multi-node tradeoff up front so users pick the right pattern. Wired into SUMMARY.md and the HPC section index.
`torc exec` is an alternative entry point, not a core workflow, so it shouldn't sit near the top of the cheat sheet. Moved it from just below Quick Start to just above Remote Workers — alongside the other specialized command groups.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies follow-up fixes to the torc exec CLI behavior and related documentation after review feedback on #271, improving correctness around ---delimited shell commands, resource monitor sampling interval handling, and standalone/TUI docs.
Changes:
- Prevent spec-file hinting for
torc exec -- <cmd ...>by gating spec detection on whether--was used, and add a regression test. - Fix
--sample-interval-secondsoverflow by switching the CLI arg type toi32and removing the lossy cast when buildingResourceMonitorConfig. - Update/reorder docs and navigation, including a new HPC guide for running self-contained Slurm jobs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_exec_cmd.rs |
Adds regression test ensuring torc exec -- ... workflow.yaml is treated as a shell command, not a spec hint. |
src/exec_cmd.rs |
Skips spec-file hinting when -- is used; updates sampling interval type and removes as i32 cast. |
src/cli.rs |
Changes --sample-interval-seconds parsing from u32 to i32 with >= 1 range enforcement. |
docs/src/specialized/hpc/self-contained-slurm-job.md |
Adds a new guide for running workflows inside a single Slurm allocation via standalone mode. |
docs/src/specialized/hpc/index.md |
Links the new self-contained Slurm job guide from the HPC index. |
docs/src/introduction.md |
Reorders Key Features list to avoid leading with torc exec. |
docs/src/core/reference/cli-cheatsheet.md |
Moves the Inline Commands section lower in the cheatsheet for better flow. |
docs/src/core/how-to/run-inline-commands.md |
Updates TUI invocation example away from -s tui. |
docs/src/core/how-to/index.md |
Reorders how-to index entries so inline commands aren’t first. |
docs/src/SUMMARY.md |
Reorders summary entries and adds the new HPC page. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
402
to
406
| .as_ref() | ||
| .map(|j| j.granularity.clone()) | ||
| .unwrap_or(MonitorGranularity::Summary), | ||
| sample_interval_seconds: sample_interval_seconds.map(|v| v as i32).unwrap_or(10), | ||
| sample_interval_seconds: sample_interval_seconds.unwrap_or(10), | ||
| generate_plots, |
There was a problem hiding this comment.
sample_interval_seconds is now Option<i32> and is written into ResourceMonitorConfig without validation. Because the resource monitor later converts this to a Duration via an as u64 cast, negative values (or 0) coming from non-CLI call sites could produce an extremely large interval / unexpected behavior. Consider validating sample_interval_seconds (>= 1) inside exec_cmd::run/build_spec before populating the spec, and returning a user-facing error if it’s invalid.
Reject `sample_interval_seconds < 1` at the single construction chokepoint. Defends against hand-written workflow specs where the value is 0 (busy-loop) or negative (wraps to a ~584-billion-year Duration via the `as u64` cast in the monitoring loop). The CLI already rejects invalid values at parse time, so this only affects deserialized-spec paths. Caller in job_runner logs and falls back to no-monitoring on failure, which is the same behavior as other resource_monitor setup errors. Added two unit tests: accept 1, reject 0/-1/i32::MIN.
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
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
Follow-ups on @pesap's review of #271 (which was merged before the comments could be addressed):
torc exec -- <cmd>vs spec-file hint —detect_spec_file_in_trailingruns before the--delimiter branch, sotorc exec -- cat workflow.yamlwas getting steered towardtorc run. Gated the check on!args.shell_command_delimitedand added a regression test (exec_delimited_spec_like_arg_is_allowed).--sample-interval-secondsoverflow — the CLI parsed it asu32with range1..and then cast toi32when buildingResourceMonitorConfig, which wraps negative for values abovei32::MAX. Switched the arg toi32(clap range1..) and dropped theas i32cast.ResourceMonitorConfig::sample_interval_secondsis alreadyi32, so this just removes the lossy conversion.torc -s tuidoc — top-level-sis ignored for thetuisubcommand (it's excluded fromrequires_server, somain.rsprints "has no effect … ignoring"). Changed the example totorc tui --standalone, which is the actual flag ontui_runner::Args.Also moved the
Run Inline Commands (torc exec)entry out of the first slot inSUMMARY.md,how-to/index.md, and introduction Key Features.torc execis an alternative entry point rather than the main torc workflow, so it shouldn't lead these lists.Test plan
cargo fmt --checkcargo clippy --all --all-targets --all-features -- -D warningsdprint checkcargo nextest run --lib(265 passed)cargo nextest run --test test_exec_cmdforexec_delimited_spec_like_arg_is_allowed,exec_spec_file_trailing_arg_suggests_torc_run,exec_non_delimited_trailing_arg_is_rejected(3 passed)