feat(spurctld): add job submission validation hook (shell + lua)

[yansun1996]

Summary

Adds a site-controlled job-submission hook — a Spur equivalent of Slurm's job_submit plugin interface — with two backends:

• Shell (hooks.job_submit): a script gets the resolved spec as JSON on stdin and decides via exit code + stdout (Spur-native).
• Lua (hooks.job_submit_lua): a sandboxed script defines slurm_job_submit(job_desc, submit_uid) and mutates job_desc in place — literal parity with Slurm's job_submit/lua, so an existing Slurm Lua policy ports with minimal changes.

Both run on the controller at submission (on the leader, before the job enters the Raft log) and can accept, reject (message shown to the user), or modify the job. If both are configured, shell runs first, then Lua, each on the evolving spec (mirroring Slurm's config-ordered plugin chain). Example policies: examples/hooks/job_submit.sh, examples/hooks/job_submit.lua.

The eight items in the request (check user/group/partition/GPUs, enforce walltime, auto-add QoS, add constraints, reject bad jobs, audit) are all expressible as site policy in either backend.

Approach

• Whitelisted modify. Both backends may only change policy/scheduling fields: qos, partition, account, constraint, comment, reservation, priority, time_limit (minutes), begin_time, gres, hold. Changes are routed through one typed parser, so identity (user/uid/gid), the script/argv, and resource-count fields are structurally unmodifiable — a policy script cannot forge identity or alter what runs. Non-whitelisted keys are ignored + logged (shell) / never read (Lua).
• Lua sandbox. The interpreter runs with only table/string/math/utf8/coroutine; os, io, package/require, and debug are excluded, and the always-loaded base globals that reach the filesystem or bytecode loader (dofile, loadfile, load, loadstring, collectgarbage) are removed. A memory ceiling and an instruction-count interrupt guard against a runaway policy hanging or OOMing the controller. time_limit is exposed and accepted in minutes (Slurm convention); unset fields read as Lua nil.
• Hook point. Runs in submit_job after default/QoS/account resolution and validation, before the size check and Raft propose, so edits are what get persisted/scheduled.
• Fail-closed. A shell hook emitting unparseable JSON, a wrong-typed field, a Lua syntax/runtime error, a missing slurm_job_submit, or an exceeded resource limit all reject the submission rather than silently accepting it.
• Post-modify re-validation. A hook-changed partition/account is re-checked against ACLs; a hook-set QoS is trusted (not re-authorized) but still existence-checked so an unknown QoS can't silently resolve to the limitless default.
• Audit logging. Each decision (accept / reject / modified fields) is logged under a stable audit target with user/uid/partition/gpu context, tagged per backend (job_submit / job_submit_lua).
• Upgrade-safe. The new config fields are additive/optional and not part of any persisted Raft/WAL type; proto is unchanged. Lua is vendored (Lua 5.4 built from source) so there is no system-Lua build dependency.

Known limitations

• If a hook sets gres GPU entries alongside a user's explicit --gpus, the requests can conflict; since GPU demand is resolved later, this surfaces at schedule time, not submit.
• The shell example requires jq; if absent it fails closed (all submissions rejected) — noted in the script header.

Test plan

• Unit tests for both backends: accept / reject (message surfaced) / modify / non-whitelisted-field-ignored / every whitelisted field / fail-closed (malformed shell JSON, wrong type, Lua syntax/runtime error, missing entry point). Lua adds: sandbox denies os/io/require/dofile/loadfile/load, infinite loop is interrupted, time_limit minutes + whole-valued float, unchanged fields not reported, unset field reads as nil. Lua also audits non-whitelisted fields a script set (ignored-key parity with shell).
• Controller wiring tests: shell and Lua reject/modify; modify to invalid partition and unknown QoS both rejected; shell→Lua chain; shell reject short-circuits Lua; Lua overrides a shell-set field.
• Native-host e2e (tests/native_host/e2e/test_job_submit_hook.py) for both backends: reject reaches the CLI, modify persists and is queryable via scontrol, unconfigured hook is inert.
• cargo clippy --workspace --exclude spur-ffi --all-targets clean; cargo fmt --all --check clean; targeted spur-core/spurctld suites pass.
• Validated end-to-end on an isolated deployment (both backends): reject surfaces the script's message; auto-QoS / priority / comment / walltime-cap modifies persist and show in scontrol; unknown-QoS rejected; unconfigured hook inert; shell malformed-JSON fails closed. Lua specifically: os.execute and dofile fail closed with no filesystem effect, an infinite loop is interrupted in well under a second while the controller stays responsive, and the shell→Lua chain applies both edits with distinct audit lines.

Design notes

• Hook-set QoS is trusted policy. A QoS set by the hook is checked for existence but not re-run through the per-user allow-list ACL (unlike a user-supplied --qos). The hook is the policy authority, so it can grant a QoS the user could not request directly.
• time_limit encoding differs by backend. The shell hook receives the resolved spec verbatim, where time_limit is a [seconds, nanos] array; the Lua hook receives it as integer minutes (Slurm job_submit.lua convention). Both example scripts show the correct handling.
• Over-cap time_limit pends, not rejects. A hook may raise time_limit above a partition's max; like an over-cap user submission, the job stays pending (the partition max-time check is a schedule-time gate), rather than being rejected at submit.

Addresses SPUR-62.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.89878% with 47 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
+ Coverage   76.24%   76.57%   +0.33%     
==========================================
  Files         166      167       +1     
  Lines       65418    66564    +1146     
==========================================
+ Hits        49877    50970    +1093     
- Misses      15541    15594      +53     

🚀 New features to boost your workflow:

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

[Copilot]

Pull request overview

Adds a site-configurable job submission hook to spurctld (implemented as an external script) so clusters can enforce and/or mutate policy at submit-time before the job is persisted into the Raft log.

Changes:

• Wire hooks.job_submit into controller submission flow, applying a whitelisted set of spec edits and surfacing hook rejections to the caller.
• Introduce a core hook runner and change-application helpers for the submit hook (stdin JSON contract, stdout change parsing).
• Add documentation/examples plus unit + native-host e2e coverage for accept/reject/modify behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/native_host/e2e/test_job_submit_hook.py	New native-host e2e coverage for reject/modify/inert behavior through the CLI→controller path.
examples/spur.conf	Documents the new optional [hooks].job_submit configuration.
examples/hooks/job_submit.sh	Provides an example policy hook script demonstrating reject + modify patterns.
crates/spurctld/src/cluster.rs	Runs the submit hook during submit_job and applies hook-directed changes.
crates/spur-core/src/hooks.rs	Implements submit-hook execution, stdout parsing into typed changes, and spec mutation helpers.
crates/spur-core/src/config.rs	Adds HooksConfig.job_submit as an additive optional config field.

Suppressed comments (2)

crates/spur-core/src/hooks.rs:257

• time_limit_minutes currently accepts negative (and extremely large) integers. A negative value will create a negative chrono::Duration later, which can bypass partition max-time checks (tl.num_minutes() > max) and lead to nonsensical scheduling behavior. Consider validating this field as a non-negative, non-overflowing minute count and failing closed when invalid.

            "reservation" => changes.reservation = Some(take_string(key, value)?),
            "priority" => changes.priority = Some(take_u32(key, value)?),
            "time_limit_minutes" => changes.time_limit_minutes = Some(take_i64(key, value)?),
            "begin_time" => changes.begin_time = Some(take_datetime(key, value)?),

crates/spur-core/src/hooks.rs:244

• The context string "job_submit hook emitted unparseable JSON" is misleading when stdout is valid JSON but not an object (e.g., a number/array). Since serde_json::from_str here expects an object/map, the error should reflect the expected shape to make hook misconfigurations easier to diagnose.

fn parse_submit_changes(stdout: &str) -> anyhow::Result<SubmitHookChanges> {
    let map: serde_json::Map<String, serde_json::Value> =
        serde_json::from_str(stdout).context("job_submit hook emitted unparseable JSON")?;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

crates/spur-core/src/hooks.rs:346

• LUA_INSTRUCTION_LIMIT is set to 100,000,000 instructions. Even though this bounds infinite loops, it can still translate to multi-second CPU stalls per submission on slower hardware, and submit_job runs synchronously on the async gRPC worker thread. Consider lowering the instruction budget (or additionally enforcing a wall-clock timeout) to keep a misbehaving policy from degrading controller availability.

/// Instruction budget before a lua script is interrupted (guards infinite loops).
const LUA_INSTRUCTION_LIMIT: u32 = 100_000_000;

crates/spur-core/src/hooks.rs:315

• job_submit changes allow time_limit_minutes to be negative or extremely large. A negative value can bypass later max_time_minutes checks (because tl.num_minutes() becomes negative), and a very large value risks overflow when it is converted into a chrono::Duration. Since hooks are expected to be fail-closed on bad input, validate time_limit_minutes during parsing (both shell + Lua paths share take_i64).

fn take_i64(key: &str, value: &serde_json::Value) -> anyhow::Result<i64> {
    // Lua arithmetic yields floats (`x / 2`), so accept a whole-valued float too.
    if let Some(f) = value.as_f64() {
        if value.as_i64().is_none() && f.fract() == 0.0 && f.is_finite() {
            return Ok(f as i64);

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

crates/spur-core/src/hooks.rs:254

• On timeout the hook process is killed but never awaited/reaped. On Unix this can leave a zombie process behind until spurctld exits, and repeated timeouts could exhaust the process table. After killing the child, wait for it (best-effort, with a short timeout) before bailing.

        Err(_) => {
            let _ = child.kill().await;
            anyhow::bail!(
                "job_submit hook timed out after {SUBMIT_HOOK_TIMEOUT_SECS}s (script: {script_path})"
            );
        }

crates/spurctld/src/cluster.rs:408

• run_submit_hook_lua does file IO + potentially heavy CPU work (Lua execution/instruction budget) but is called directly from submit_job, which is typically executed on a Tokio worker thread via the gRPC/REST handlers. Unlike the shell path, this can block the runtime and reduce controller throughput. Consider running the Lua hook inside block_in_place (or spawn_blocking) as well, matching the shell hook’s scheduling behavior.

        if let Some(script) = lua {
            let ctx = self.submit_hook_ctx(spec)?;
            let outcome = spur_core::hooks::run_submit_hook_lua(script, &ctx)
                .map_err(|e| SubmitError::internal(format!("job_submit lua hook failed: {e}")))?;
            self.apply_submit_outcome(spec, "job_submit_lua", outcome)?;

[biluriuday]

Thanks for the PR. Making the whitelist a typed struct rather than a string set means identity and resource fields are unmodifiable by construction rather than by a check someone can forget — that's the correct security architecture for this feature. Routing both backends through a single changes_from_map means the two paths can't drift on type validation. The sandbox goes past the obvious os/io removal to strip the always-loaded base-library filesystem and bytecode globals, which is the step most implementations miss. The post-modify re-validation of partition/account ACLs, with the deliberate and documented exception for hook-set QoS, is the right trust model. And the test density is high — the "unchanged field must not be reported as a change" cases in particular are the ones people usually skip and then regret.

Blocking

1. Lua leaks into every binary in the workspace, including the FFI shared object.

mlua (vendored Lua 5.4 C source) was added to spur-core, and ten crates depend on spur-core:

$ cargo tree -p spur-ffi -i mlua
mlua v0.12.0
└── spur-core v0.7.0
└── spur-ffi v0.7.0
spur-ffi is crate-type = ["cdylib", "staticlib"]. That means a Lua interpreter now ships inside libspur_compat.so/.a, plus spurd and spur (the CLI), none of which will ever run a submit hook. Three consequences, in descending severity:

Any consumer statically linking libspur_compat.a that already links Lua gets duplicate-symbol link failures. This is not hypothetical — Slurm links Lua for job_submit/lua, and Slurm-adjacent processes are precisely the FFI target audience.
A C toolchain is now a hard build requirement for every target, including cross/musl builds and the k8s image build.
Binary size and attack surface on compute nodes for a controller-only feature.
Fix: put the Lua backend behind a cargo feature (spur-core/lua, default off, enabled only by spurctld), or move it out of spur-core entirely — the hook is controller policy, so crates/spurctld/src/hooks/lua.rs is arguably the more honest home. I couldn't build to confirm the symbol export empirically (the build was blocked), so treat the linker claim as a risk to verify with nm -D, but the dependency edge itself is confirmed.

2. The hook runs before the submission size check.

cluster.rs
Lines 409-434
pub fn submit_job(&self, mut spec: JobSpec) -> Result<JobId, SubmitError> {
...
self.run_job_submit_hook(&mut spec)?;
// Checked after defaults are applied so we measure the final spec.
// Array expansion only adds bounded integer metadata per task, so a
// single pre-expansion check still bounds each Raft log entry.
check_submission_size(&spec)?;
MAX_JOB_SPEC_SIZE is 4 MiB, but that gate is downstream of the hook. So a spec that will be rejected anyway first gets serialized to JSON, piped to a forked shell process, and — on the Lua path — re-parsed by serde_json and materialized into a Lua table under a 64 MiB ceiling. Any authenticated user can amplify a doomed submission into a fork plus tens of MiB of controller allocation. Move check_submission_size(&spec)? above run_job_submit_hook; the comment's premise (measure the final spec) is about defaults, not about hook edits, and hook edits are bounded whitelist fields.

3. The Lua hook blocks the async runtime; the shell hook doesn't.

The shell path correctly uses block_in_place, matching the convention propose() already established in this file. The Lua path calls straight through:

cluster.rs
Lines 487-492
if let Some(script) = lua {
let ctx = self.submit_hook_ctx(spec)?;
let outcome = spur_core::hooks::run_submit_hook_lua(script, &ctx)
.map_err(|e| SubmitError::internal(format!("job_submit lua hook failed: {e}")))?;
self.apply_submit_outcome(spec, "job_submit_lua", outcome)?;
}
That's a synchronous std::fs::read_to_string plus up to 100M Lua instructions on a Tokio worker thread. Copilot flagged this and it's correct. Related: LUA_INSTRUCTION_LIMIT is an instruction rate trigger, not a time bound — instruction cost varies by two orders of magnitude across opcodes, so 100M could be 200 ms or 20 s. Wrap the call in block_in_place for parity with the shell path, and add an actual wall-clock deadline (the hook callback can check Instant::elapsed) rather than relying on instruction count alone. I'd also drop the budget to ~5M; policy logic doesn't need 100M.

4. The rejection reason is the hook's entire stderr.

hooks.rs
Lines 266-274
if !status.success() {
let reason = stderr_text.trim();
let reason = if reason.is_empty() {
format!("job rejected by job_submit hook (exit {status})")
} else {
reason.to_string()
};
return Ok(SubmitHookOutcome::Reject(reason));
}
Up to 1 MiB of stderr becomes a gRPC InvalidArgument message. Two problems: stderr is simultaneously the log stream (every line is warn!-ed above) and the user-facing reason, so one set -x or one shellcheck warning turns into the rejection text; and a large message risks tripping gRPC message/metadata limits, converting a clean policy rejection into a transport error. Cap the reason (4 KiB, or the last N lines) and consider a dedicated channel — a {"reject": "..."} object on stdout, or a SPUR_HOOK_MESSAGE_FD — so stderr can stay a pure log stream.

Important
5. The shell contract is fragile and becomes permanent on merge. "Exit 0 with any stdout means modify, and non-JSON stdout is a hard failure" means a single stray echo in a site script fails every submission cluster-wide with an Internal error. Once sites have scripts in production, changing this is a breaking change. I'd spend the extra hour now on a versioned envelope — {"version": 1, "action": "accept|reject|modify", "changes": {...}, "message": "..."} — which also gives you a natural place to put the rejection message from item 4 and room to add outcomes later.

6. Nothing validates the hook path until the first submission. A typo in hooks.job_submit is discovered when a user submits, and then every submission fails with an opaque Internal error. Validate at config load and on reconfigure: absolute (already done, but at run time), exists, regular file, executable for the shell backend; for Lua, actually load and compile it. Decide explicitly whether a bad hook means "refuse to start" or "start with the hook disabled and a loud warning," and document it.

7. Hook script permissions are unchecked — this is a root-execution surface. spurctld runs as root, and the Lua design note says a hook-set QoS is trusted policy that bypasses the per-user ACL. So whoever can write the hook file can grant any QoS and, for the shell backend, execute arbitrary code as root. The absolute-path requirement is a good start but only closes $PATH hijacking. Slurm checks ownership/permissions on Prolog/Epilog and lua files; do the same here — refuse a hook that isn't root-owned or that is group/world-writable. Flagging per the repo's standing instruction to surface security issues.

8. The Lua script is re-read, re-parsed, and re-VM'd on every submission. Beyond the per-submit cost (vendored Lua init plus compile), an operator editing the file in place makes every concurrent submission fail on a partially-written script. Compile once at startup/reconfigure and cache. Keep a fresh VM per call if you want isolation, but the source shouldn't come off disk on the hot path.

9. Policy is enforced at submit only — scontrol update walks right around it. update_job accepts time_limit, priority, partition, comment, account, and qos, and runs its own ACL checks but never the hook. A site policy like "cap walltime at 24 h" or "force qos=low for account X" is undone with one scontrol update. Slurm's plugin interface has job_modify() for exactly this reason, so this is also a parity gap. Either wire the hook into update_job (a follow-up PR is fine) or state the gap prominently in examples/spur.conf and both example scripts, because operators will otherwise assume they have an enforcement boundary they don't have.

10. The gres/gpus conflict is avoidable, not inherent. The PR lists it as a known limitation that surfaces at schedule time. But resolve_gpu_demand_for already returns Err on conflict — that's the fallback branch inside effective_gpus. Call it directly after apply_submit_changes when changes.gres.is_some() and reject at submit. About five lines, and it converts a silently-stuck job into a clear error.

11. Shell and Lua disagree on clearing a field. In lua_table_to_changes, a null value hits if json.is_null() { continue; }, so job_desc.reservation = nil is a silent no-op. On the shell side {"reservation": ""} does clear it (only qos/partition/account filter empty strings, deliberately). A Slurm Lua policy being ported that does job_desc.reservation = nil will silently do nothing. Pick one semantics, document it in both example script headers, and at minimum log when a Lua script nils a whitelisted field.

12. The two bounds added in the last commit have no tests. The 30 s timeout and the 1 MiB output cap are the fail-closed guarantees the whole design rests on, and there's no coverage for either. Worth noting the cap doesn't currently produce a "output too large" error — read_capped leaves the excess unread, the child blocks on a full pipe, and the operator gets a timeout 30 s later. That's a confusing diagnostic for a distinct failure mode; give it its own error and test both.

13. HA operational sharp edge. The hook runs on the leader only (correct — server.rs forwards to the leader, and the hook runs pre-propose so the modified spec is what replicates). But hook config is per-node file config, so drift across the three controllers means policy silently changes on failover. Not a code blocker, but it belongs in the docs, and a startup log line stating the active hook paths and a hash of each script would make drift diagnosable.

Items 1, 2, 3, and 4 are what I'd gate on; 5 and 7 are the ones I'd argue hardest for doing before this ships rather than after, since both get more expensive once sites have scripts in production.

[yansun1996]

Thanks for the thorough review — this was really useful. Summary of what changed, grouped by land-now vs. documented-gap vs. follow-up.

Addressed in this PR:

1. mlua leaking into every binary. Moved the Lua backend out of spur-core into spurctld (crates/spurctld/src/hooks.rs); spur-core no longer depends on mlua at all. The shared outcome/whitelist/apply types and the shell backend stay in spur-core. cargo tree -p spur-ffi -i mlua now returns nothing — the interpreter and its vendored C ship only with the controller. Agreed this is the more honest home for controller policy.
2. Size check after the hook. Moved check_submission_size above run_job_submit_hook. The hook only edits bounded whitelist fields, so this still measures the persisted spec.
3. Lua blocking the runtime. Wrapped the Lua call in block_in_place, matching the shell path. (Instruction budget and a true wall-clock deadline left as a follow-up — see below.)
4. Reject reason = entire stderr. The user-facing reason is now capped (~4 KiB, tail kept); full stderr still goes to the log.
5. No validation until first submission. Added a validate_submit_hooks step at startup and on reconfigure: absolute + secure + executable regular file for the shell hook, and a compile check for the Lua hook. A bad hook now fails loudly at startup instead of on the first submit.
6. Unchecked script permissions. The runner now refuses a hook that isn't root/controller-owned or is group/world-writable, on both backends.
7. gres/gpus conflict. Now calls resolve_gpu_demand_for after applying hook changes when gres is set, and rejects at submit with a clear message instead of deferring to schedule time.
8. Untested bounds + confusing cap diagnostic. read_capped now drains to EOF (so the child never blocks) and flags overflow, and the runner returns a distinct "output exceeded N bytes" error rather than a misleading timeout. Added unit coverage for the timeout, the output cap, and the reason cap.

Documented (examples/spur.conf + both example headers), not silently dropped:

9. scontrol update bypasses the hook. Real gap; wiring a modify-time hook is a larger change I'd rather do separately. Noted prominently that enforcement is submit-only.
10. shell ""-clears vs Lua nil-noop. Documented the asymmetry in both example headers.
11. HA per-node hook drift. Documented the operational note (keep the script identical across controllers).

Deferred to a follow-up:

5. Versioned stdout envelope. Agree it's the right long-term contract; it's a bigger design change and I'd rather not rush it into this PR.
   3b. Lua wall-clock deadline + lower instruction budget. The block_in_place wrap addresses the runtime-blocking half now; a true time bound is the follow-up.
6. Compile-once Lua cache. The compile check at load closes the "bad script discovered late" hole; caching the compiled source on the hot path is a follow-up.

All local tests plus the new coverage pass. Thanks again.