From 97730e59a1aa513e444d11fe700d2996394b19d5 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 5 Jul 2026 14:04:00 -0400 Subject: [PATCH] fix(#1164): wire external-job config keys + document wave:post choice (#2006) Refinements A-D to the external-job capability (PR #1998 follow-up): A. Document why the contribution registers at execute:wave:post: #1164 asks for wave:pre, but execute-phase.md only dispatches wave:post today (wave:pre is declared in the loop host contract but not rendered). Wiring wave:pre is a core-loop change #1164 puts out of scope; the executor honors the runtime_budget classification guidance before running any tagged task. B. external_job.artifact_dir is now consumed (was declared but unused): the adapter resolves it via the canonical capability-config seam and surfaces the resolved root in submit output. C. external_job.submit_timeout_ms / poll_timeout_ms are now read from config (were shadowed by env-only reads). Precedence: env > config > registry default; non-numeric config values fall back (no guessing, no NaN). D. CLI surface gains unit coverage: parseFlags, findPlanningDir, resolveExternalJobSettings, formatShowReport. Regenerates capability-registry.cjs from the updated capability.json. --- .changeset/external-job-config-wiring.md | 5 + capabilities/external-job/capability.json | 8 +- .../fragments/execute-wave-post.md | 10 +- docs/reference/long-running-operations.md | 38 +++- gsd-core/bin/lib/capability-registry.cjs | 18 +- scripts/slurm-adapter.cjs | 108 +++++++++-- tests/slurm-adapter.test.cjs | 171 ++++++++++++++++++ 7 files changed, 326 insertions(+), 32 deletions(-) create mode 100644 .changeset/external-job-config-wiring.md create mode 100644 tests/slurm-adapter.test.cjs diff --git a/.changeset/external-job-config-wiring.md b/.changeset/external-job-config-wiring.md new file mode 100644 index 000000000..8b06a03cd --- /dev/null +++ b/.changeset/external-job-config-wiring.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2006 +--- +**Setting `external_job.submit_timeout_ms` / `poll_timeout_ms` / `artifact_dir` in `.planning/config.json` now actually configures the SLURM adapter** — the keys were declared by the external-job capability but the adapter only read env vars, so config edits silently had no effect. The adapter now resolves them through the canonical capability-config seam (env override > config > registry default), surfaces the resolved `artifact_dir` in `submit` output, documents why the contribution registers at `execute:wave:post` (#1164 asks for `wave:pre`, which `execute-phase.md` does not dispatch today; wiring it is a core-loop change #1164 explicitly defers), and gains unit coverage for the CLI surface (`parseFlags`, `findPlanningDir`, `resolveExternalJobSettings`, `formatShowReport`). (#1164) diff --git a/capabilities/external-job/capability.json b/capabilities/external-job/capability.json index 170294288..cf1a85aca 100644 --- a/capabilities/external-job/capability.json +++ b/capabilities/external-job/capability.json @@ -3,7 +3,7 @@ "role": "feature", "version": "1.7.0-rc.2", "title": "Async external-job scheduler adapter", - "description": "Default-off producer of the async external-job manifest (#1164). At execute:wave:post an executor can externalize long-running compute (SLURM first, scheduler-pluggable), commit a .planning/async-jobs/.json manifest, defer SUMMARY.md, and return external_job_waiting. The core loop (#1165) consumes the manifest; this capability is the only thing that writes it.", + "description": "Default-off producer of the async external-job manifest (#1164). At execute:wave:post an executor can externalize long-running compute (SLURM first, scheduler-pluggable), commit a .planning/async-jobs/.json manifest, defer SUMMARY.md, and return external_job_waiting. The core loop (#1165) consumes the manifest; this capability is the only thing that writes it. NOTE on contribution point: #1164 specifies execute:wave:pre, but execute-phase.md only dispatches execute:wave:post today (wave:pre is declared in the loop host contract but not rendered); wiring wave:pre dispatch is a core-loop change #1164 explicitly puts out of scope, so this capability registers at wave:post and the executor honors the runtime_budget classification guidance before running any tagged task. The adapter (scripts/slurm-adapter.cjs) reads external_job.submit_timeout_ms / poll_timeout_ms / artifact_dir through the canonical capability-config seam (env override > config > registry default).", "tier": "full", "requires": [], "engines": { @@ -35,17 +35,17 @@ "external_job.artifact_dir": { "type": "string", "default": "Artifacts/jobs", - "description": "Root for per-job artifact directories (e.g. Artifacts/jobs//). Avoids fixed log paths and hardcoding a cluster/project layout." + "description": "Root for per-job artifact directories (e.g. Artifacts/jobs//). Avoids fixed log paths and hardcoding a cluster/project layout. Surfaced by the adapter (`slurm-adapter.cjs submit` prints the resolved value); override via GSD_EXTERNAL_JOB_ARTIFACT_DIR." }, "external_job.submit_timeout_ms": { "type": "number", "default": 30000, - "description": "Hard timeout (ms) for the scheduler submit subprocess (e.g. sbatch). Bounded per CLAUDE.md unbounded-subprocess policy." + "description": "Hard timeout (ms) for the scheduler submit subprocess (e.g. sbatch). Bounded per CLAUDE.md unbounded-subprocess policy. Read by the adapter (env GSD_SLURM_SUBMIT_TIMEOUT_MS overrides)." }, "external_job.poll_timeout_ms": { "type": "number", "default": 15000, - "description": "Hard timeout (ms) for the scheduler poll subprocess (squeue, with sacct fallback)." + "description": "Hard timeout (ms) for the scheduler poll subprocess (squeue, with sacct fallback). Read by the adapter (env GSD_SLURM_POLL_TIMEOUT_MS overrides)." } }, "steps": [], diff --git a/capabilities/external-job/fragments/execute-wave-post.md b/capabilities/external-job/fragments/execute-wave-post.md index 8f132807d..d6cbd57f2 100644 --- a/capabilities/external-job/fragments/execute-wave-post.md +++ b/capabilities/external-job/fragments/execute-wave-post.md @@ -1,4 +1,12 @@ - + ## Externalize long-running compute (async external job) diff --git a/docs/reference/long-running-operations.md b/docs/reference/long-running-operations.md index 40dc064d3..366c96b94 100644 --- a/docs/reference/long-running-operations.md +++ b/docs/reference/long-running-operations.md @@ -21,6 +21,16 @@ Every executable task carries a runtime budget. Planners emit it via a `` element (taught by the `external-job` Capability's `plan:post` fragment); executors branch on it at `execute:wave:post`. +> **Contribution point.** `#1164` specifies classification at +> `execute:wave:pre`, but `execute-phase.md` only dispatches +> `execute:wave:post` today — `wave:pre` is declared in the loop host contract +> but not rendered. Wiring `wave:pre` dispatch is a core-loop change `#1164` +> explicitly puts out of scope ("without touching core loop semantics"), so the +> Capability registers at `execute:wave:post` and the executor honors the +> classification guidance **before** running any task tagged +> `long_compute`, whether in the current or a +> subsequent wave. + | Budget | Meaning | Execute behavior | |---|---|---| | `quick` | Under ~2 min | Run normally in the foreground. | @@ -93,7 +103,33 @@ state→manifest-status mapping, manifest build/validate, `sbatch`/`squeue`/ behind a single module; a new backend adds a sibling state map and parser without touching core. -## 7. Related +## 7. Configuration + +The `external-job` Capability declares its config keys in +[`capability.json`](../../capabilities/external-job/capability.json) and the +`slurm-adapter` resolves them through the canonical capability-config seam +(`resolveConfigKey` in `capability-activation.cjs`). **Precedence: env override +> nested config value > registry default.** + +| Key | Default | Env override | Read by | +|---|---|---|---| +| `external_job.enabled` | `false` | — | Capability gate (`when` on both contributions). Master toggle; default-off. | +| `external_job.backend` | `slurm` | — | Pluggability seam (LSF/PBS/K8s future). Core never interprets it. | +| `external_job.artifact_dir` | `Artifacts/jobs` | `GSD_EXTERNAL_JOB_ARTIFACT_DIR` | Adapter surfaces the resolved root in `submit` output. | +| `external_job.submit_timeout_ms` | `30000` | `GSD_SLURM_SUBMIT_TIMEOUT_MS` | Adapter bounds the `sbatch` subprocess. | +| `external_job.poll_timeout_ms` | `15000` | `GSD_SLURM_POLL_TIMEOUT_MS` | Adapter bounds the `squeue`/`sacct` subprocess. | + +Config keys live nested under `external_job` in `.planning/config.json`, e.g.: + +```json +{ "external_job": { "enabled": true, "submit_timeout_ms": 45000 } } +``` + +The timeouts are load-bearing for the bounded-subprocess policy: the adapter +never unbounds a scheduler subprocess, and a non-numeric config value falls back +to the registry default rather than producing `NaN` (no guessing). + +## 8. Related - **How-To:** [`../how-to/async-external-jobs.md`](../how-to/async-external-jobs.md) — using the SLURM adapter. - **Contract:** [`planning-artifacts.md`](./planning-artifacts.md) — the manifest stability contract. diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index 6dfb625ac..54888b319 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -961,7 +961,7 @@ const capabilities = { "role": "feature", "version": "1.7.0-rc.2", "title": "Async external-job scheduler adapter", - "description": "Default-off producer of the async external-job manifest (#1164). At execute:wave:post an executor can externalize long-running compute (SLURM first, scheduler-pluggable), commit a .planning/async-jobs/.json manifest, defer SUMMARY.md, and return external_job_waiting. The core loop (#1165) consumes the manifest; this capability is the only thing that writes it.", + "description": "Default-off producer of the async external-job manifest (#1164). At execute:wave:post an executor can externalize long-running compute (SLURM first, scheduler-pluggable), commit a .planning/async-jobs/.json manifest, defer SUMMARY.md, and return external_job_waiting. The core loop (#1165) consumes the manifest; this capability is the only thing that writes it. NOTE on contribution point: #1164 specifies execute:wave:pre, but execute-phase.md only dispatches execute:wave:post today (wave:pre is declared in the loop host contract but not rendered); wiring wave:pre dispatch is a core-loop change #1164 explicitly puts out of scope, so this capability registers at wave:post and the executor honors the runtime_budget classification guidance before running any tagged task. The adapter (scripts/slurm-adapter.cjs) reads external_job.submit_timeout_ms / poll_timeout_ms / artifact_dir through the canonical capability-config seam (env override > config > registry default).", "tier": "full", "requires": [], "engines": { @@ -993,17 +993,17 @@ const capabilities = { "external_job.artifact_dir": { "type": "string", "default": "Artifacts/jobs", - "description": "Root for per-job artifact directories (e.g. Artifacts/jobs//). Avoids fixed log paths and hardcoding a cluster/project layout." + "description": "Root for per-job artifact directories (e.g. Artifacts/jobs//). Avoids fixed log paths and hardcoding a cluster/project layout. Surfaced by the adapter (`slurm-adapter.cjs submit` prints the resolved value); override via GSD_EXTERNAL_JOB_ARTIFACT_DIR." }, "external_job.submit_timeout_ms": { "type": "number", "default": 30000, - "description": "Hard timeout (ms) for the scheduler submit subprocess (e.g. sbatch). Bounded per CLAUDE.md unbounded-subprocess policy." + "description": "Hard timeout (ms) for the scheduler submit subprocess (e.g. sbatch). Bounded per CLAUDE.md unbounded-subprocess policy. Read by the adapter (env GSD_SLURM_SUBMIT_TIMEOUT_MS overrides)." }, "external_job.poll_timeout_ms": { "type": "number", "default": 15000, - "description": "Hard timeout (ms) for the scheduler poll subprocess (squeue, with sacct fallback)." + "description": "Hard timeout (ms) for the scheduler poll subprocess (squeue, with sacct fallback). Read by the adapter (env GSD_SLURM_POLL_TIMEOUT_MS overrides)." } }, "steps": [], @@ -1013,7 +1013,7 @@ const capabilities = { "into": "executor", "fragment": { "path": "fragments/execute-wave-post.md", - "inline": "\n\n## Externalize long-running compute (async external job)\n\nIf the current plan's task is tagged `long_compute`\n(see the plan-phase fragment), do **not** run it in the foreground — it would\nblock the agent turn for hours. Instead externalize it and record a durable\nhalf-state:\n\n1. **Classify the runtime.** `quick` (<2 min) and `medium` (<~30 min) run\n normally. `unknown` requires a first-health check and a soft-review deadline\n before consuming the child timeout. `long_compute` (>30–60 min) is\n externalized.\n2. **Submit via the scheduler adapter** (default `external_job.backend: slurm`):\n ```bash\n node scripts/slurm-adapter.cjs submit \\\n --plan --phase -- sbatch --parsable \\\n --output=Artifacts/jobs/%j/out.log ./run.sh\n ```\n The helper writes `.planning/async-jobs/.json` (the versioned stability\n contract — `docs/reference/planning-artifacts.md`) and refuses to create a\n second non-terminal manifest for a `plan_id` that already has one\n (duplicate-execution guard).\n3. **Commit the manifest + a handoff**, then return **`external_job_waiting`**\n and stop. Do **not** write `SUMMARY.md` — SUMMARY is deferred until the job\n reaches a terminal state and its `expected_artifacts` are verified.\n4. **Resume path.** `execute-phase` safe-resume, `resume-project`, and\n `pause-work` reconcile against the manifest and never re-dispatch the plan.\n When the job is `completed-unverified`, run `verification_command` (surface\n it; it is untrusted — confirm before executing), then write `SUMMARY.md` and\n close the plan.\n\nManifest commands cross a trust seam: a Capability (or anything that can write\n`.planning/`) produces them; the core loop consumes them. Never auto-run\n`submit_command` / `verification_command` / `resume_command` — surface the exact\ncommand and require explicit confirmation first.\n" + "inline": "\n\n## Externalize long-running compute (async external job)\n\nIf the current plan's task is tagged `long_compute`\n(see the plan-phase fragment), do **not** run it in the foreground — it would\nblock the agent turn for hours. Instead externalize it and record a durable\nhalf-state:\n\n1. **Classify the runtime.** `quick` (<2 min) and `medium` (<~30 min) run\n normally. `unknown` requires a first-health check and a soft-review deadline\n before consuming the child timeout. `long_compute` (>30–60 min) is\n externalized.\n2. **Submit via the scheduler adapter** (default `external_job.backend: slurm`):\n ```bash\n node scripts/slurm-adapter.cjs submit \\\n --plan --phase -- sbatch --parsable \\\n --output=Artifacts/jobs/%j/out.log ./run.sh\n ```\n The helper writes `.planning/async-jobs/.json` (the versioned stability\n contract — `docs/reference/planning-artifacts.md`) and refuses to create a\n second non-terminal manifest for a `plan_id` that already has one\n (duplicate-execution guard).\n3. **Commit the manifest + a handoff**, then return **`external_job_waiting`**\n and stop. Do **not** write `SUMMARY.md` — SUMMARY is deferred until the job\n reaches a terminal state and its `expected_artifacts` are verified.\n4. **Resume path.** `execute-phase` safe-resume, `resume-project`, and\n `pause-work` reconcile against the manifest and never re-dispatch the plan.\n When the job is `completed-unverified`, run `verification_command` (surface\n it; it is untrusted — confirm before executing), then write `SUMMARY.md` and\n close the plan.\n\nManifest commands cross a trust seam: a Capability (or anything that can write\n`.planning/`) produces them; the core loop consumes them. Never auto-run\n`submit_command` / `verification_command` / `resume_command` — surface the exact\ncommand and require explicit confirmation first.\n" }, "produces": [ ".planning/async-jobs/.json" @@ -2757,7 +2757,7 @@ const byLoopPoint = { "into": "executor", "fragment": { "path": "fragments/execute-wave-post.md", - "inline": "\n\n## Externalize long-running compute (async external job)\n\nIf the current plan's task is tagged `long_compute`\n(see the plan-phase fragment), do **not** run it in the foreground — it would\nblock the agent turn for hours. Instead externalize it and record a durable\nhalf-state:\n\n1. **Classify the runtime.** `quick` (<2 min) and `medium` (<~30 min) run\n normally. `unknown` requires a first-health check and a soft-review deadline\n before consuming the child timeout. `long_compute` (>30–60 min) is\n externalized.\n2. **Submit via the scheduler adapter** (default `external_job.backend: slurm`):\n ```bash\n node scripts/slurm-adapter.cjs submit \\\n --plan --phase -- sbatch --parsable \\\n --output=Artifacts/jobs/%j/out.log ./run.sh\n ```\n The helper writes `.planning/async-jobs/.json` (the versioned stability\n contract — `docs/reference/planning-artifacts.md`) and refuses to create a\n second non-terminal manifest for a `plan_id` that already has one\n (duplicate-execution guard).\n3. **Commit the manifest + a handoff**, then return **`external_job_waiting`**\n and stop. Do **not** write `SUMMARY.md` — SUMMARY is deferred until the job\n reaches a terminal state and its `expected_artifacts` are verified.\n4. **Resume path.** `execute-phase` safe-resume, `resume-project`, and\n `pause-work` reconcile against the manifest and never re-dispatch the plan.\n When the job is `completed-unverified`, run `verification_command` (surface\n it; it is untrusted — confirm before executing), then write `SUMMARY.md` and\n close the plan.\n\nManifest commands cross a trust seam: a Capability (or anything that can write\n`.planning/`) produces them; the core loop consumes them. Never auto-run\n`submit_command` / `verification_command` / `resume_command` — surface the exact\ncommand and require explicit confirmation first.\n" + "inline": "\n\n## Externalize long-running compute (async external job)\n\nIf the current plan's task is tagged `long_compute`\n(see the plan-phase fragment), do **not** run it in the foreground — it would\nblock the agent turn for hours. Instead externalize it and record a durable\nhalf-state:\n\n1. **Classify the runtime.** `quick` (<2 min) and `medium` (<~30 min) run\n normally. `unknown` requires a first-health check and a soft-review deadline\n before consuming the child timeout. `long_compute` (>30–60 min) is\n externalized.\n2. **Submit via the scheduler adapter** (default `external_job.backend: slurm`):\n ```bash\n node scripts/slurm-adapter.cjs submit \\\n --plan --phase -- sbatch --parsable \\\n --output=Artifacts/jobs/%j/out.log ./run.sh\n ```\n The helper writes `.planning/async-jobs/.json` (the versioned stability\n contract — `docs/reference/planning-artifacts.md`) and refuses to create a\n second non-terminal manifest for a `plan_id` that already has one\n (duplicate-execution guard).\n3. **Commit the manifest + a handoff**, then return **`external_job_waiting`**\n and stop. Do **not** write `SUMMARY.md` — SUMMARY is deferred until the job\n reaches a terminal state and its `expected_artifacts` are verified.\n4. **Resume path.** `execute-phase` safe-resume, `resume-project`, and\n `pause-work` reconcile against the manifest and never re-dispatch the plan.\n When the job is `completed-unverified`, run `verification_command` (surface\n it; it is untrusted — confirm before executing), then write `SUMMARY.md` and\n close the plan.\n\nManifest commands cross a trust seam: a Capability (or anything that can write\n`.planning/`) produces them; the core loop consumes them. Never auto-run\n`submit_command` / `verification_command` / `resume_command` — surface the exact\ncommand and require explicit confirmation first.\n" }, "produces": [ ".planning/async-jobs/.json" @@ -3076,19 +3076,19 @@ const configSchema = { "owner": "external-job", "type": "string", "default": "Artifacts/jobs", - "description": "Root for per-job artifact directories (e.g. Artifacts/jobs//). Avoids fixed log paths and hardcoding a cluster/project layout." + "description": "Root for per-job artifact directories (e.g. Artifacts/jobs//). Avoids fixed log paths and hardcoding a cluster/project layout. Surfaced by the adapter (`slurm-adapter.cjs submit` prints the resolved value); override via GSD_EXTERNAL_JOB_ARTIFACT_DIR." }, "external_job.submit_timeout_ms": { "owner": "external-job", "type": "number", "default": 30000, - "description": "Hard timeout (ms) for the scheduler submit subprocess (e.g. sbatch). Bounded per CLAUDE.md unbounded-subprocess policy." + "description": "Hard timeout (ms) for the scheduler submit subprocess (e.g. sbatch). Bounded per CLAUDE.md unbounded-subprocess policy. Read by the adapter (env GSD_SLURM_SUBMIT_TIMEOUT_MS overrides)." }, "external_job.poll_timeout_ms": { "owner": "external-job", "type": "number", "default": 15000, - "description": "Hard timeout (ms) for the scheduler poll subprocess (squeue, with sacct fallback)." + "description": "Hard timeout (ms) for the scheduler poll subprocess (squeue, with sacct fallback). Read by the adapter (env GSD_SLURM_POLL_TIMEOUT_MS overrides)." }, "workflow.post_planning_gaps": { "owner": "gap-analysis", diff --git a/scripts/slurm-adapter.cjs b/scripts/slurm-adapter.cjs index 944d7313c..82c8ee99d 100644 --- a/scripts/slurm-adapter.cjs +++ b/scripts/slurm-adapter.cjs @@ -28,8 +28,54 @@ const path = require('node:path'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); const m = require('../gsd-core/bin/lib/external-job.cjs'); -const SUBMIT_TIMEOUT_MS = Number(process.env.GSD_SLURM_SUBMIT_TIMEOUT_MS || 30000); -const POLL_TIMEOUT_MS = Number(process.env.GSD_SLURM_POLL_TIMEOUT_MS || 15000); +// #1164 refinements B + C: the adapter now resolves external_job.* settings +// through the canonical capability-config seam (resolveConfigKey in +// capability-activation.cjs), which walks loadConfig -> workstream config.json +// -> root config.json -> registry configSchema default. Env vars remain the +// top-precedence override so cluster operators can tune without editing config. +const { resolveConfigKey } = require('../gsd-core/bin/lib/capability-activation.cjs'); +const FALLBACK_SUBMIT_TIMEOUT_MS = 30000; +const FALLBACK_POLL_TIMEOUT_MS = 15000; +const FALLBACK_ARTIFACT_DIR = 'Artifacts/jobs'; + +/** + * Resolve the external_job.* runtime settings. Pure given {config, env, + * registry}; degrades to hardcoded fallbacks if the registry/config are absent + * so the adapter never unbounds a subprocess (CLAUDE.md bounded-subprocess). + * + * Precedence: env override > nested config value > registry default > fallback. + */ +function resolveExternalJobSettings({ cwd, env, config, registry } = {}) { + const reg = registry || {}; + let cfg = config; + if (cfg === undefined) { + try { + cfg = require('../gsd-core/bin/lib/config-loader.cjs').loadConfig(cwd); + } catch { + cfg = {}; // resolveConfigKey still falls back to the registry default + } + } + const e = env || {}; + const submitEnv = e.GSD_SLURM_SUBMIT_TIMEOUT_MS; + const pollEnv = e.GSD_SLURM_POLL_TIMEOUT_MS; + const artifactEnv = e.GSD_EXTERNAL_JOB_ARTIFACT_DIR; + const submit = submitEnv ? Number(submitEnv) + : _num(resolveConfigKey('external_job.submit_timeout_ms', { config: cfg, cwd, registry: reg }), FALLBACK_SUBMIT_TIMEOUT_MS); + const poll = pollEnv ? Number(pollEnv) + : _num(resolveConfigKey('external_job.poll_timeout_ms', { config: cfg, cwd, registry: reg }), FALLBACK_POLL_TIMEOUT_MS); + const artifactDir = artifactEnv + || _str(resolveConfigKey('external_job.artifact_dir', { config: cfg, cwd, registry: reg }), FALLBACK_ARTIFACT_DIR); + return { submitTimeoutMs: submit, pollTimeoutMs: poll, artifactDir }; +} + +function _num(res, fallback) { + const v = res && res.found ? res.value : undefined; + return typeof v === 'number' && Number.isFinite(v) ? v : fallback; +} +function _str(res, fallback) { + const v = res && res.found ? res.value : undefined; + return typeof v === 'string' && v.length > 0 ? v : fallback; +} function usage() { return [ @@ -82,11 +128,18 @@ function cmdSubmit(flags) { const resume = flags.resume || ('/gsd:execute-phase ' + phase); if (!verify) throw new ExitError(1, 'submit requires --verify (the command that verifies job output)'); + // Resolve settings from config/env before any subprocess (env > config > default). + const planningDir = findPlanningDir(); + const settings = resolveExternalJobSettings({ + cwd: path.dirname(planningDir), + env: process.env, + }); + let stdout; try { stdout = execFileSync(sbatchCmd[0], sbatchCmd.slice(1), { encoding: 'utf8', - timeout: SUBMIT_TIMEOUT_MS, + timeout: settings.submitTimeoutMs, maxBuffer: 1024 * 1024, }); } catch (e) { @@ -107,13 +160,13 @@ function cmdSubmit(flags) { verification_command: verify, resume_command: resume, }); - const planningDir = findPlanningDir(); const res = m.writeManifest(manifest, planningDir); if (!res.ok) { throw new ExitError(1, 'writeManifest refused (' + res.kind + '): ' + res.message); } process.stdout.write('submitted job ' + parsed.job_id + ' for plan ' + plan + '\n'); process.stdout.write('manifest: ' + res.path + '\n'); + process.stdout.write('artifact_dir: ' + settings.artifactDir + '\n'); process.stdout.write('state: external_job_waiting (SUMMARY deferred)\n'); } @@ -121,6 +174,10 @@ function cmdPoll(flags) { const jobId = flags.job; if (!jobId) throw new ExitError(1, 'poll requires --job \n' + usage()); const planningDir = findPlanningDir(); + const settings = resolveExternalJobSettings({ + cwd: path.dirname(planningDir), + env: process.env, + }); const manifestFile = m.manifestPath(planningDir, jobId); if (!fs.existsSync(manifestFile)) { throw new ExitError(1, 'no manifest for job ' + jobId + ' at ' + manifestFile); @@ -130,7 +187,7 @@ function cmdPoll(flags) { let rawState = null; try { const out = execFileSync('squeue', ['-h', '-j', jobId, '-o', '%i %T'], { - encoding: 'utf8', timeout: POLL_TIMEOUT_MS, maxBuffer: 1024 * 1024, + encoding: 'utf8', timeout: settings.pollTimeoutMs, maxBuffer: 1024 * 1024, }).trim(); const line = out.split('\n')[0]; const parsed = m.parseSqueueLine(line || ''); @@ -139,7 +196,7 @@ function cmdPoll(flags) { if (!rawState) { try { const out = execFileSync('sacct', ['-X', '-P', '-j', jobId, '-o', 'JobID,State'], { - encoding: 'utf8', timeout: POLL_TIMEOUT_MS, maxBuffer: 1024 * 1024, + encoding: 'utf8', timeout: settings.pollTimeoutMs, maxBuffer: 1024 * 1024, }).trim(); for (const line of out.split('\n').slice(1)) { const parsed = m.parseSacctRow(line.split('|')); @@ -161,6 +218,22 @@ function cmdPoll(flags) { process.stdout.write(JSON.stringify({ job_id: jobId, slurm_state: rawState, manifest_status: mapped, path: res.path }) + '\n'); } +function formatShowReport(manifest) { + const lines = []; + lines.push('job ' + manifest.job_id + ' (plan ' + manifest.plan_id + ', backend ' + manifest.backend + ')'); + lines.push('status: ' + manifest.status); + if (manifest.terminal_details) { + lines.push('terminal_details: ' + JSON.stringify(manifest.terminal_details)); + } + // Trust boundary: surface commands for confirmation, never auto-run. + lines.push(''); + lines.push('Manifest commands (UNTRUSTED — confirm before running):'); + lines.push(' submit_command: ' + manifest.submit_command); + lines.push(' verification_command: ' + manifest.verification_command); + lines.push(' resume_command: ' + manifest.resume_command); + return lines.join('\n') + '\n'; +} + function cmdShow(flags) { const jobId = flags.job; if (!jobId) throw new ExitError(1, 'show requires --job \n' + usage()); @@ -170,16 +243,7 @@ function cmdShow(flags) { throw new ExitError(1, 'no manifest for job ' + jobId + ' at ' + manifestFile); } const manifest = JSON.parse(fs.readFileSync(manifestFile, 'utf8')); - process.stdout.write('job ' + manifest.job_id + ' (plan ' + manifest.plan_id + ', backend ' + manifest.backend + ')\n'); - process.stdout.write('status: ' + manifest.status + '\n'); - if (manifest.terminal_details) { - process.stdout.write('terminal_details: ' + JSON.stringify(manifest.terminal_details) + '\n'); - } - // Trust boundary: surface commands for confirmation, never auto-run. - process.stdout.write('\nManifest commands (UNTRUSTED — confirm before running):\n'); - process.stdout.write(' submit_command: ' + manifest.submit_command + '\n'); - process.stdout.write(' verification_command: ' + manifest.verification_command + '\n'); - process.stdout.write(' resume_command: ' + manifest.resume_command + '\n'); + process.stdout.write(formatShowReport(manifest)); } function main() { @@ -192,4 +256,14 @@ function main() { return 0; } -runMain(main); +if (require.main === module) { + runMain(main); +} + +module.exports = { + parseFlags, + findPlanningDir, + resolveExternalJobSettings, + formatShowReport, + ExitError, +}; diff --git a/tests/slurm-adapter.test.cjs b/tests/slurm-adapter.test.cjs new file mode 100644 index 000000000..38a162015 --- /dev/null +++ b/tests/slurm-adapter.test.cjs @@ -0,0 +1,171 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +// Refinements for issue #1164 (PR #1998 follow-up): +// - A: document the execute:wave:post choice (wave:pre is declared but not +// dispatched by execute-phase.md; wiring it is a core-loop change #1164 +// puts out of scope). +// - B: external_job.artifact_dir is now consumed by the adapter (was declared +// but unused). +// - C: external_job.submit_timeout_ms / poll_timeout_ms are now read from +// config (were shadowed by env-only reads). +// - D: CLI surface (parseFlags, findPlanningDir, resolveExternalJobSettings, +// formatShowReport) now has unit coverage. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const fs = require('node:fs'); +const os = require('node:os'); + +const { + parseFlags, + findPlanningDir, + resolveExternalJobSettings, + formatShowReport, + ExitError, +} = require('../scripts/slurm-adapter.cjs'); + +// Minimal registry stand-in: only configSchema defaults are consulted by +// resolveConfigKey Level 4 (capability-activation.cjs). Real registry comes +// from gsd-core/bin/lib/capability-registry.cjs at runtime. +function fakeRegistry() { + return { + configSchema: { + 'external_job.submit_timeout_ms': { owner: 'external-job', type: 'number', default: 30000 }, + 'external_job.poll_timeout_ms': { owner: 'external-job', type: 'number', default: 15000 }, + 'external_job.artifact_dir': { owner: 'external-job', type: 'string', default: 'Artifacts/jobs' }, + }, + }; +} + +// ─── parseFlags ─────────────────────────────────────────────────────────────── + +test('parseFlags collects --key value pairs and a `--` rest array', () => { + const f = parseFlags(['--plan', '3.1', '--phase', '3', '--', 'sbatch', '--parsable', './run.sh']); + assert.strictEqual(f.plan, '3.1'); + assert.strictEqual(f.phase, '3'); + assert.deepStrictEqual(f['--'], ['sbatch', '--parsable', './run.sh']); + assert.deepStrictEqual(f._, []); +}); + +test('parseFlags collects positional args into _', () => { + const f = parseFlags(['submit', '--job', '123']); + assert.deepStrictEqual(f._, ['submit']); + assert.strictEqual(f.job, '123'); +}); + +test('parseFlags handles a missing `--` rest gracefully (no rest array)', () => { + const f = parseFlags(['--job', '123']); + assert.strictEqual(f.job, '123'); + assert.strictEqual(f['--'], undefined); +}); + +// ─── findPlanningDir ────────────────────────────────────────────────────────── + +test('findPlanningDir walks up to the nearest .planning and returns its path', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-slurm-')); + const planning = path.join(root, '.planning'); + fs.mkdirSync(planning); + const nested = path.join(root, 'a', 'b', 'c'); + fs.mkdirSync(nested, { recursive: true }); + assert.strictEqual(findPlanningDir(nested), planning); +}); + +test('findPlanningDir fails closed (ExitError) when no .planning is reachable', () => { + // A tmp dir with no .planning anywhere up to the walk bound (10 levels). + // Use a fresh tmp and create 11 nested dirs so the walk can't escape to a + // parent that happens to contain .planning (e.g. the repo root). + const deep = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-slurm-noplan-')); + let cur = deep; + for (let i = 0; i < 12; i++) { + cur = path.join(cur, `n${i}`); + fs.mkdirSync(cur); + } + assert.throws(() => findPlanningDir(cur), (e) => e instanceof ExitError && e.code === 1); +}); + +// ─── resolveExternalJobSettings (#1164 refinements B + C) ───────────────────── + +test('resolveExternalJobSettings falls back to registry defaults when config and env are empty', () => { + const s = resolveExternalJobSettings({ config: {}, env: {}, registry: fakeRegistry() }); + assert.strictEqual(s.submitTimeoutMs, 30000); + assert.strictEqual(s.pollTimeoutMs, 15000); + assert.strictEqual(s.artifactDir, 'Artifacts/jobs'); +}); + +test('resolveExternalJobSettings reads nested config values (config key now functional, was unused)', () => { + const config = { + external_job: { + submit_timeout_ms: 45000, + poll_timeout_ms: 9000, + artifact_dir: 'Artifacts/hpc', + }, + }; + const s = resolveExternalJobSettings({ config, env: {}, registry: fakeRegistry() }); + assert.strictEqual(s.submitTimeoutMs, 45000, 'submit_timeout_ms from config'); + assert.strictEqual(s.pollTimeoutMs, 9000, 'poll_timeout_ms from config'); + assert.strictEqual(s.artifactDir, 'Artifacts/hpc', 'artifact_dir from config'); +}); + +test('resolveExternalJobSettings: env override beats config (precedence env > config > default)', () => { + const config = { external_job: { submit_timeout_ms: 45000, poll_timeout_ms: 9000 } }; + const env = { + GSD_SLURM_SUBMIT_TIMEOUT_MS: '7777', + GSD_SLURM_POLL_TIMEOUT_MS: '8888', + GSD_EXTERNAL_JOB_ARTIFACT_DIR: 'Artifacts/env', + }; + const s = resolveExternalJobSettings({ config, env, registry: fakeRegistry() }); + assert.strictEqual(s.submitTimeoutMs, 7777, 'env submit wins'); + assert.strictEqual(s.pollTimeoutMs, 8888, 'env poll wins'); + assert.strictEqual(s.artifactDir, 'Artifacts/env', 'env artifact_dir wins'); +}); + +test('resolveExternalJobSettings degrades to hardcoded defaults when registry is absent and config empty', () => { + // Defensive: if a caller passes no registry (e.g. a minimal embed), the + // adapter must still produce sane timeouts (CLAUDE.md bounded-subprocess). + const s = resolveExternalJobSettings({ config: {}, env: {} }); + assert.strictEqual(s.submitTimeoutMs, 30000); + assert.strictEqual(s.pollTimeoutMs, 15000); + assert.strictEqual(s.artifactDir, 'Artifacts/jobs'); +}); + +test('resolveExternalJobSettings ignores a non-numeric config value (no guessing — falls back)', () => { + const config = { external_job: { submit_timeout_ms: 'not-a-number' } }; + const s = resolveExternalJobSettings({ config, env: {}, registry: fakeRegistry() }); + // Bad config value must not produce NaN that would unbound execFileSync. + assert.ok(Number.isFinite(s.submitTimeoutMs), 'submit must stay finite'); + assert.strictEqual(s.submitTimeoutMs, 30000, 'fell back to registry default'); +}); + +// ─── formatShowReport (trust boundary: surface commands, never auto-run) ────── + +test('formatShowReport surfaces job identity, status, terminal_details, and the UNTRUSTED command hint', () => { + const manifest = { + job_id: '12345', + plan_id: '3.1', + backend: 'slurm', + status: 'completed-unverified', + terminal_details: { reason: 'ok', exit_code: 0 }, + submit_command: 'sbatch --parsable ./run.sh', + verification_command: 'python -m verify.py 12345', + resume_command: '/gsd:execute-phase 3', + }; + const out = formatShowReport(manifest); + assert.match(out, /job 12345 \(plan 3\.1, backend slurm\)/); + assert.match(out, /status: completed-unverified/); + assert.match(out, /terminal_details:/); + assert.match(out, /UNTRUSTED — confirm before running/); + assert.match(out, /verification_command: python -m verify\.py 12345/); +}); + +test('formatShowReport omits the terminal_details line when none are present', () => { + const manifest = { + job_id: '1', plan_id: '1', backend: 'slurm', status: 'running', + submit_command: 's', verification_command: 'v', resume_command: 'r', + terminal_details: null, + }; + const out = formatShowReport(manifest); + assert.doesNotMatch(out, /terminal_details:/, 'no details line when null'); + assert.match(out, /status: running/); +});