From 2e068f8d6ca1246050d7b138e4fd876ec98804da Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Mon, 3 Aug 2026 15:35:25 -0400 Subject: [PATCH 01/15] feat(session-tools): add slurm-sizing and slurm-digest skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port the personal ~/.claude Slurm sizing system into two session-tools skills. slurm-sizing consults a measured usage table before a job is submitted and logs what each job actually ran; slurm-digest merges a weekly usage digest into that table. Enforcement is the skill description (trigger-heavy, wrapper-aware) since a plugin cannot write to a user's CLAUDE.md. User data (table, log, digest archive) is not shipped — it lives under the user's own ~/.claude/slurm-sizing/ and is located via a config contract (reference/config.md) with no default for digest_cluster, since guessing the wrong cluster silently corrupts sizing advice (job IDs are not unique across clusters). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jjcp9DNrdrmFxDadgCFNrC --- .claude-plugin/marketplace.json | 2 +- README.md | 8 + .../session-tools/.claude-plugin/plugin.json | 4 +- .../skills/slurm-digest/SKILL.md | 230 ++++++++++++++++++ .../skills/slurm-sizing/SKILL.md | 105 ++++++++ .../skills/slurm-sizing/reference/config.md | 98 ++++++++ 6 files changed, 444 insertions(+), 3 deletions(-) create mode 100644 plugins/session-tools/skills/slurm-digest/SKILL.md create mode 100644 plugins/session-tools/skills/slurm-sizing/SKILL.md create mode 100644 plugins/session-tools/skills/slurm-sizing/reference/config.md diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index b9f049d..1b10284 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -16,7 +16,7 @@ { "name": "session-tools", "source": "./plugins/session-tools", - "description": "Session productivity tools for Claude Code. Includes self-assess for auditing sessions for errors, inefficiencies, and repeated workarounds." + "description": "Session productivity tools for Claude Code. Includes self-assess for auditing sessions for errors, inefficiencies, and repeated workarounds, plus Slurm job sizing from measured usage instead of habit." } ] } diff --git a/README.md b/README.md index 1989113..191c1aa 100644 --- a/README.md +++ b/README.md @@ -8,9 +8,17 @@ A shared plugin marketplace for [Claude Code](https://code.claude.com/) at White |--------|-------|--------------| | [`fry-python-tool`](plugins/fry-python-tool/) | `fry-python-tool` | Find existing GPU-powered computational biology tools on the Whitehead fry cluster, or scaffold a new one. For single-purpose Python tools that produce outputs (embeddings, segmentations, predictions) for downstream analysis. | | [`session-tools`](plugins/session-tools/) | `self-assess` | Audit the current Claude Code session for errors, inefficiencies, and repeated workarounds, then produce actionable recommendations to improve skills, scripts, CLAUDE.md, and tooling. | +| [`session-tools`](plugins/session-tools/) | `slurm-sizing` | Loads automatically before and after Slurm job submission. Sizes `--mem`/`--cpus-per-task` from a table of measured past usage instead of habit, and logs what each new job ran so a later `slurm-digest` merge can attach real scope to it. | +| [`session-tools`](plugins/session-tools/) | `slurm-digest` | User-invoked. Merges a weekly Slurm usage digest into the measured sizing table `slurm-sizing` reads from. | After installing (see below), invoke a skill with `/whitehead:` — e.g., `/whitehead:self-assess`. +`slurm-sizing`'s data — the usage table and job log it reads and writes — lives under your own +`~/.claude/slurm-sizing/`. It is **not shipped with the plugin** and starts empty; it becomes +useful once you've merged your first digest with `slurm-digest`. A recommendation is only +actionable when the workload scope behind the measurement is known: rows with an unknown scope +are marked as lower bounds and can justify *raising* a request, but never *lowering* one. + ## Using This Marketplace ### Add the marketplace diff --git a/plugins/session-tools/.claude-plugin/plugin.json b/plugins/session-tools/.claude-plugin/plugin.json index 4d7e3cf..03b3ea0 100644 --- a/plugins/session-tools/.claude-plugin/plugin.json +++ b/plugins/session-tools/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "session-tools", - "description": "Session productivity tools for Claude Code. Includes self-assess for auditing sessions for errors, inefficiencies, and repeated workarounds.", - "version": "1.0.0", + "description": "Session productivity tools for Claude Code. Includes self-assess for auditing sessions for errors, inefficiencies, and repeated workarounds, plus slurm-sizing/slurm-digest for sizing Slurm jobs from measured usage instead of habit.", + "version": "1.1.0", "author": { "name": "Whitehead Institute" } diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md new file mode 100644 index 0000000..e72296b --- /dev/null +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -0,0 +1,230 @@ +--- +name: slurm-digest +description: >- + Merge a weekly Slurm usage digest into the sizing table used by slurm-sizing. User-invoked via + /session-tools:slurm-digest — paste the week's usage digest as the argument and run it + explicitly; this skill does not trigger on its own the way slurm-sizing does. +user-invocable: true +allowed-tools: Read, Write, Edit, Bash(cat *), Bash(ls *), Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(join *), Bash(sacct *), Bash(sacctmgr *), Bash(hostname) +--- + +# Slurm Digest Merge + +Full config contract (keys, defaults, bootstrap file formats, and the multi-user filtering rules +referenced in Step 3 below): `../slurm-sizing/reference/config.md`. Read `digest_cluster`, +`digest_user`, `table`, `log`, `archive`, `multi_user_digest`, and `policy` from +`~/.claude/slurm-sizing/config.json` before anything else below; if that config doesn't exist +yet, follow its "Bootstrap" section first rather than guessing any of these values. + +Merge the Slurm digest pasted below into the sizing table at the configured `table` path +(default `~/.claude/slurm-sizing/table.md`). + +## Procedure + +1. **Refuse duplicates (precondition — check this FIRST, before parsing or merging anything).** + Determine the digest's date and check whether `/YYYY-MM-DD.tsv` already exists for it, + where `` is the configured `archive` path (default `~/.claude/slurm-sizing/digests`). + **If it does, STOP here: change nothing** — do not parse, join, merge, or write anything — + report that the digest is already archived for that date, and ask whether to force. Re-merging + an already-counted digest would inflate `n` (nine `score` runs would read as eighteen), and `n` + is the signal that says whether a recommendation is trustworthy. Only proceed past this step + if no file exists for that date, or the user has explicitly instructed you to force a + re-merge. + +2. **Parse.** Read the pasted table. Columns are `User JobID JobName ReqMem(M) UsedMem(M) + MemPct ReqCPU UsedCPU CPUPct Elapsed`. Skip the `OVERALL` line, skip `bash`, `python3`, and + any bare shell or interpreter name. + +3. **Filter to `digest_user` — a consent-gated filtering procedure, not part of parsing.** + Merge only rows whose `User` matches the configured `digest_user` (default: the invoking + `$USER`). Another person's peak for a same-named job would corrupt this user's recommendation + with no error anywhere, so this filter runs before anything downstream sees a row. Precisely + what to do when other users' rows appear — ask once, then remember: + - **First time** (`multi_user_digest` is unset or `false` in config) **and other users are + present:** HALT before merging anything. Report which other users are present and how many + rows each has, and ask whether this is a shared digest that should be filtered to + `digest_user`. + - **On confirmation:** write `"multi_user_digest": true` into the config, then proceed, + filtering to `digest_user` and reporting the filtered-out count each week thereafter. Do not + halt again. + - **If `multi_user_digest` is already `true`:** filter and report, never halt. + - **If the user declines** (this is not a shared digest to filter): merge nothing and stop. Do + NOT write `multi_user_digest`. Report the mismatch and suggest checking that `digest_user` + names the right account and that the pasted digest is the intended one. This condition + re-halts on the next run by design — nothing was resolved, so proceeding silently would be + worse than asking again. + - **Never** filter silently on a first encounter, and **never** merge another user's rows at + all, under any branch above. + - If no row matches `digest_user`, say so explicitly rather than silently merging nothing. + +4. **Exclude capped runs.** Drop any row with `MemPct >= 99`. Its MaxRSS is a ceiling imposed + by `--mem`, not a measured peak. Note in the report how many rows were dropped this way. + +5. **Join for scope, on `(cluster, jobid)` — never `jobid` alone.** The log at the configured + `log` path (default `~/.claude/slurm-sizing/jobs.tsv`) begins with `#`-prefixed comment lines + before its header row — **filter them first**: run `grep -v '^#' ` before running + `awk`/`join` on that file. **This join is carried out with real shell commands, not by + reasoning over `Read` output** — the failure mode below is what `join` itself does, and only + manifests when it is actually invoked: treating the comments as data feeds `join` unsorted + input (it will warn "is not sorted") and can produce false non-matches. The filtered log's + header is + `cluster jobid submitted job_name scope cwd`. + **Digests come from the configured `digest_cluster` only; the log's rows carry their own + `cluster` value, which may name a different cluster (e.g. `cluster-b`) for jobs submitted + elsewhere.** `digest_cluster` and other clusters are separate Slurm ID namespaces — the same + `jobid` number can label unrelated jobs on each one — so a bare `jobid` join is a correctness + bug: it will silently splice an unrelated cluster's job onto this digest's scope and produce a + confidently wrong recommendation. Joining on `jobid` alone is never acceptable, even as a + shortcut. Before matching, **exclude every log row whose `cluster` is not the digest's own + `digest_cluster`** — those rows are not eligible matches regardless of `jobid` equality. For + each remaining digest row, look up `JobID` among the rows that survived that exclusion. A + match supplies the workload scope. No match means `scope: unknown`. **If zero rows match + against `digest_cluster` log rows and `digest_cluster` rows exist in the log, say so + prominently** — a silently failing join makes every recommendation a lower bound and the + system quietly stops working. (A zero-match result is *expected*, not a defect, when the + filtered log contains no `digest_cluster` rows at all — e.g. it currently holds only rows + whose provenance is some other cluster.) + +6. **Enrich unmatched rows from `sacct` — a conditional recovery procedure, not part of the + join.** For every digest row that did NOT match a submission-log entry in Step 5, attempt to + recover a *derived* scope directly from Slurm before falling back to `scope: unknown`. The six + bullets below are sequential and order-dependent — cluster guard, then batch query, then + discard step rows, then collapse array rows, then the miss guard, then record — treat them as + an ordered procedure to follow in full, not as loose elaboration on the join. + - **Cluster guard, checked first.** `sacct` only sees the LOCAL cluster's accounting + database — **verified in practice**: a job ID from the digest's cluster does not resolve + via `sacct` run against a different cluster's accounting database (`sacct -L -j ` + returns nothing for it there), and `sacctmgr -n -P list cluster format=Cluster` lists only + the local cluster's name. Determine the local cluster (e.g. `sacctmgr -n -P list cluster + format=Cluster` or `hostname`) and compare it to the digest's cluster (`digest_cluster` from + config). If they differ, **skip enrichment entirely** for the whole digest — do not query + `sacct` — and say so plainly in the report (Step 12). Never fabricate a scope to compensate. + - **Batch query, one field per call — never a combined multi-field query.** Query once for + every unmatched row, not per row, but query `SubmitLine` and `WorkDir` in **separate** calls: + `sacct -j --format=JobID,SubmitLine --parsable2 --noheader` and, only + if `WorkDir` is wanted, `sacct -j --format=JobID,WorkDir --parsable2 + --noheader`. **Do not combine fields into one `--format=JobID,SubmitLine,WorkDir` call and do + not "fix" this with `--delimiter`.** `--parsable2` separates fields with `|`, and a submit + line can itself contain a `|` (e.g. an ordinary `--wrap="zcat x | seqkit stats"`), which + silently turns one 3-field row into a 5-field row no fixed-column parse recovers — verified + live: a combined `JobID,SubmitLine,WorkDir` query interleaves `WorkDir` into the wrong + position whenever a wrapped command uses one or more pipes. No replacement delimiter is safe + either, since any character you pick could itself appear in a submit line. Querying one field + at a time makes each row exactly `JobID|` — a `JobID` can never contain `|`, so + splitting on the **first** `|` only is exact regardless of what the field holds. Keep this as + two calls; do not re-merge them into one query. + - **Discard step rows before treating a hit as real.** A base job (and every array task within + it) returns extra rows for its steps — `.batch`, `.extern`, and any other + `.`-suffixed step — with empty `SubmitLine`/`WorkDir`. Drop any returned row whose `JobID` + contains a `.` before doing anything else with the result; only bare (``) or + array-task (`_`) rows carry real data. + - **Array jobs: map task rows back to the digest's base job, then collapse to one scope.** A + digest row for an array job has a bare base `JobID` (e.g. `7932156`); `sacct` returns one row + per task (`7932156_0` … `7932156_11`) plus that task's step rows. After discarding step rows, + strip the `_` suffix to correlate each surviving row back to the base job. The + surviving task rows for one base job carry an identical `SubmitLine` (it's the same `sbatch` + invocation) — take one as the scope. If they somehow differ, record the first and add a note + flagging the discrepancy rather than inventing a merge. This case is exactly where the + feature earns its keep: a 12-task array charged 32G/task is the scale signal enrichment + exists to recover, and it is silently lost if task/step rows aren't collapsed first. + - **Empty-result / retention / absent-jobid guard.** Three things all count as the same miss — + record nothing and leave the row `scope: unknown`: (a) `sacct` errors or is unavailable; + (b) a jobid returns a row with an empty `SubmitLine`; (c) a jobid returns **no row at all** + (purged by retention, or never existed on this cluster). Do not treat any of the three as + evidence of anything. Note in the report (Step 12) how many rows missed this way. + - **Record the full `SubmitLine` verbatim, prefixed `derived:`, never bare.** Store the whole + recovered submit line as-is — do not trim it to "just the interesting flags"; the untrimmed + line is the evidence, and no extraction rule can pick out what mattered without silently + discarding information (e.g. a `--mem=` token is still worth seeing at the Step 10 prompt). + Write it into the row's `scope` column as `derived: ` — e.g. + `derived: sbatch --dependency=afterok:7932155 --array=0-11%4 --mem=32G + scripts/08_screen_zarr.sh` — visibly distinct from a declared scope (no prefix) that came + from the join. **A `derived:` scope does NOT clear the `>=` prefix on `rec_mem`** — a submit + line records what the measurement WAS, not that the next run shares that scope — so Step 8's + asymmetric scope rule still treats the row as unknown-scope for sizing purposes until a + human promotes it (Step 10). + +7. **Merge.** For each job name: + `peak_G = max(existing_peak_G, this week's max UsedMem_MB / 1024)`. + **The `/1024` is not optional.** `existing_peak_G` (the table's `peak_G` column) is already in + GB; `UsedMem` in the digest is in MB (`UsedMem(M)`, per Step 2's column header) — skip the + conversion and you corrupt `peak_G` by roughly 1000x (e.g. the first merge of a 4794.71 MB + reading would write `peak_G = 4794` where `4.7` belongs). Add to `n`, update `last_seen`. + Never lower a peak. + **Skip rows flagged `IO-BOUND`.** That flag lives in the `notes` column of the job's + **existing row in the sizing table** (the configured `table` path) — never in the incoming + digest, which carries no such marker. Check the current table row before merging, not the + pasted digest; a digest row for an IO-BOUND job looks like any other row and its MaxRSS is + page cache, not demand, so it must not enter the max. + +8. **Recompute `rec_mem`.** `rec_mem = policy.margin_multiplier x full-precision peak` (default + `margin_multiplier = 2`), where full-precision peak is `max(UsedMem_MB) / 1024` — **not** the + 1-decimal `peak_G` value displayed in the table. Floor the result at `policy.mem_floor_gb` + (default 8 G), then round UP to the next multiple of `policy.mem_round_gb` (default 4 G) — + never down; this is a safety margin, so a peak that lands exactly on a multiple of + `mem_round_gb` stays there, and any remainder pushes to the next multiple up. Worked example at + the default values: a full-precision peak of 13.2 G gives `2 x 13.2 = 26.4`, which rounds up to + 28 G, not down to 24 G. **These are the reviewed defaults, not hardcoded law** — a site that + lowers `margin_multiplier`, or any other `policy` value, in config is choosing more OOM risk in + exchange for queue priority, and should make that choice deliberately, in config, not by + editing this file. Then apply the asymmetric scope rule: + - scope matches the recorded scope -> plain recommendation, no prefix + - scope is larger than recorded -> do NOT lower; scale by the known ratio or keep the prior + request and treat this run as a fresh measurement + - scope is smaller than recorded -> a smaller scope is not evidence about the larger one; do + NOT lower anything, treat it like unknown scope for recommendation purposes, and note the + scope it was actually measured at + - scope unknown -> prefix `>=`; the row may justify raising a request, never lowering one + +9. **CPU.** One formula across the whole range — no bands, no carve-outs: + `rec_cpu = min(ReqCPU, max(policy.cpu_floor, ceil(ReqCPU x CPUPct / 100 x + policy.cpu_headroom)))` — at the default values (`cpu_floor = 2`, `cpu_headroom = 1.5`), + `rec_cpu = min(ReqCPU, max(2, ceil(ReqCPU x CPUPct/100 x 1.5)))`. **The `ReqCPU` cap is not + optional:** without the outer `min(ReqCPU, ...)`, a job at 4 requested CPUs and 90% + utilisation computes `ceil(4 x 0.9 x 1.5) = 6` — recommending more CPUs than were requested — + so the cap brings it back down to 4. The `cpu_headroom` multiplier (default 1.5x) is + deliberate headroom over the observed average, not a serial/parallel classification — e.g. + a job at 25% of 16 requested CPUs used ~4 cores on average, so `ceil(16 x 0.25 x 1.5) = 6`. + Record the observed `CPUPct` in `notes`. + +10. **Prompt for scope.** List unmatched job names ranked by wasted reservation + `(ReqMem - rec_mem) x n`, largest first, capped at 10. Ask the user to supply scope for those. + Rank by waste, not recency: scope on a 9-run 200 G job is worth ~1,584 G; scope on a one-off + 8 G job is worth nothing. Names left unanswered stay `unknown` and carry forward. + **Show derived evidence alongside the prompt.** For any unmatched name Step 6 enriched with a + `derived:` scope, display that derived submit line next to it so the human can confirm or + correct it. **Both confirmation and correction promote it identically:** whether the human + accepts the derived submit line as-is or supplies a corrected scope in its place, the result is + a declared scope — drop the `derived:` prefix, and it is now governed by Step 8's normal + asymmetric rule (eligible to have `>=` removed). It is the human's confirmation or correction + that authorizes this, never the derived string by itself. Names with no derived evidence are + prompted exactly as before. + +11. **Persist, then archive.** This is the point the on-disk table changes: write the fully + merged and recomputed table (Steps 7-10 applied) back to the configured `table` path now. + Only after that write succeeds, write the raw pasted digest to `/YYYY-MM-DD.tsv` + (the configured `archive` path) — the duplicate check already happened in Step 1, so this + write should never collide with an existing file under normal operation. + +12. **Report.** State: rows parsed, rows dropped as capped, join matches, job names updated, + job names new, peaks that rose, and the total reservation delta if the recommendations were + applied. Be explicit that `>=` rows are not actionable for sizing down. + Also state: how many unmatched rows were enriched with a `derived:` scope from `sacct`, how + many attempted enrichments missed (empty `SubmitLine` / retention gap), and whether + enrichment was skipped entirely because the digest's cluster differs from the local cluster + (the Step 6 cluster guard) — if skipped, say so explicitly rather than silently omitting the + counts. + Also state: how many rows were filtered out because their `User` did not match + `digest_user` (Step 3), whenever that filtering applied. + +## Rules + +- Advisory only. Do not edit job scripts, launcher scripts, or any pipeline code. Do not submit, + cancel, or modify jobs. +- Never lower a peak; never remove an `IO-BOUND` flag without in-process evidence + (`psutil`-style RSS sampling), because `sacct` MaxRSS cannot distinguish demand from page cache. +- If the digest's column layout differs from the above, stop and report rather than guessing. + +## Digest + +$ARGUMENTS diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md new file mode 100644 index 0000000..2bd3139 --- /dev/null +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -0,0 +1,105 @@ +--- +name: slurm-sizing +description: >- + Size Slurm jobs from measured usage instead of habit. TRIGGER — load this BEFORE writing or + running anything that submits a cluster job, and immediately AFTER submitting one (to record + what workload it ran). "Anything that submits a job" means: a literal `sbatch`/`srun`/`salloc` + command; ANY wrapper, launcher, Makefile target or shell script that submits on your behalf + (e.g. `./flow.sh --backend slurm`, `submit_*.sh`, any script containing `#SBATCH` directives, + anything whose output includes "Submitted batch job"); and any choice of + `--mem`/`--cpus-per-task`/`--time`. Also triggers on "run this on the cluster", "how much + memory should this job get", "why did my job OOM", "am I over-requesting", or any question + about Slurm resource sizing. SKIP only when no cluster job is involved at any remove. +user-invocable: true +allowed-tools: Read, Write, Edit, Bash(cat *), Bash(ls *), Bash(mkdir *), Bash(sacctmgr *), Bash(scontrol *), Bash(grep *) +--- + +# Slurm Job Sizing + +Full config contract (keys, defaults, bootstrap file formats): `reference/config.md`. Consult it +whenever any of the files below don't exist yet. + +## 1. Cluster gate — check this before anything else + +1. Read `digest_cluster` from `~/.claude/slurm-sizing/config.json` (path may be overridden in the + config itself). + - **Config file does not exist:** do not guess anything. Go to **Bootstrap** (§6) first. +2. Determine the local cluster: `sacctmgr -n -P list cluster format=Cluster`, falling back to + `scontrol show config | grep ClusterName`. + - **Both commands fail or return nothing:** the local cluster is undeterminable. Say so + explicitly and STOP — do not consult the table, do not append to the log. "Unknown" is never + treated as a match. +3. Compare the two values. + - **They match:** proceed to §2. + - **They differ:** say so explicitly (name both clusters) and STOP. Do not consult the table + and do not append to the log. This system is inert on any cluster other than the one the + digest was built from — job IDs collide across clusters (see §5), so numbers from the wrong + cluster are worse than no numbers at all. + +## 2. Before sizing any job — read the table first + +Read the table at the configured `table` path (default `~/.claude/slurm-sizing/table.md`). + +- **Table file does not exist:** go to **Bootstrap** (§6). +- **Job name is present in the table:** use that row's `rec_mem` / `rec_cpu` and state which row + you used (e.g. "using the `align_star` row: rec_mem=32G, rec_cpu=8"). + - **The value carries a `>=` prefix:** that's a lower bound from a run whose scope was never + recorded, so the true peak could be higher. It may justify *raising* the request above the + table value; it must never justify *lowering* the request below it. +- **Job name is absent from the table:** say so explicitly, then either ask what scope to expect + or start small and measure. Never fall back to a remembered/habitual number ("jobs like this + usually need 64G") — an absent row is a reason to measure, not a reason to guess. + +## 3. Never size from raw `sacct` MaxRSS on an I/O-heavy job + +`MaxRSS` on a job that reads or writes a lot of data includes kernel page cache charged to that +job's cgroup — it is not memory the job actually demanded, and sizing from it can overstate the +real requirement by a large multiple. Rows marked `IO-BOUND` in the table are hand-pinned from +in-process measurement (e.g. `resource.getrusage`, a memory profiler) instead — trust those over +any `sacct`-derived figure for the same job, and don't recompute an `IO-BOUND` row from `sacct`. + +## 4. After submitting — log what it actually ran + +Append one tab-separated row to the log (default `~/.claude/slurm-sizing/jobs.tsv`): + +``` +cluster jobid submitted job_name scope cwd +``` + +- **Log file does not exist:** go to **Bootstrap** (§6), then append the row. +- `scope` is a short, concrete description of the workload this run did — e.g. + "12 tasks x 1565 tiles (FULL)", "5 of 100 perturbations" — not a repeat of the job name. +- A peak logged with no `scope` is only ever a lower bound: once the job has finished, `sacct` has + no way to recover what it actually processed. Write the scope now, while it's known, even before + the peak usage is known. + +## 5. Why `cluster` is the first column + +Job IDs are not unique across clusters — two different clusters can each have a job `123456`. The +join key for this whole system is the pair `(cluster, jobid)`, never `jobid` alone. Putting +`cluster` first keeps that join key visible and stops one cluster's workload scope from silently +attaching to another cluster's job. + +## 6. Bootstrap — config, table, or log missing + +Full file formats and rationale: `reference/config.md` → "Bootstrap" section. The branches, so +none gets skipped: + +- **Config missing:** ask the user for `digest_cluster` — there is no safe default (§1). Before + writing a fresh file, check for a pre-plugin layout (`~/.claude/slurm-sizing.md`, + `~/.claude/slurm-jobs.tsv`, `~/.claude/slurm-digests/`); if any exist, offer to point the new + config at them instead of starting empty files beside real history. +- **Table missing:** create it with the header and explanatory prose, zero data rows. An empty + table is a valid, complete state — every job is unknown, which is a reason to measure, not a + reason to size from habit. +- **Log missing:** create it with the `(cluster, jobid)` join-key comment header, then the + tab-separated column header `cluster jobid submitted job_name scope cwd`. +- **Archive dir missing:** create it (empty). + +Report every file created during bootstrap — a user who expected existing data needs to know the +skill didn't find it, rather than silently starting fresh next to it. + +## 7. Weekly maintenance + +This table is only as good as the digests merged into it. Use `/session-tools:slurm-digest` to +merge a new weekly usage digest into the table. diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md new file mode 100644 index 0000000..be07c01 --- /dev/null +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -0,0 +1,98 @@ +# Slurm sizing — configuration + +Config lives at `~/.claude/slurm-sizing/config.json`. Both `slurm-sizing` and `slurm-digest` +read it. If it does not exist, the skills bootstrap it (below). + +```json +{ + "digest_cluster": "fry", + "digest_user": "alice", + "table": "~/.claude/slurm-sizing/table.md", + "log": "~/.claude/slurm-sizing/jobs.tsv", + "archive": "~/.claude/slurm-sizing/digests", + "multi_user_digest": false, + "policy": { + "margin_multiplier": 2, + "mem_floor_gb": 8, + "mem_round_gb": 4, + "cpu_headroom": 1.5, + "cpu_floor": 2 + } +} +``` + +| Key | Default | Meaning | +|---|---|---| +| `digest_cluster` | **none — must be set** | The Slurm cluster whose weekly usage digest feeds this system. Compared against the local cluster before anything is consulted or logged. | +| `digest_user` | the invoking `$USER` | Which user's rows to merge. The digest carries a `User` column and may contain several people's jobs. | +| `table` | `~/.claude/slurm-sizing/table.md` | The sizing table. | +| `log` | `~/.claude/slurm-sizing/jobs.tsv` | The submission log where scope is recorded. | +| `archive` | `~/.claude/slurm-sizing/digests` | Directory of raw archived digests, one per date. | +| `multi_user_digest` | `false` | This digest is known to contain other users' rows and should be filtered without halting. | +| `policy.margin_multiplier` | `2` | `rec_mem` = this × peak. | +| `policy.mem_floor_gb` | `8` | Never recommend less than this. | +| `policy.mem_round_gb` | `4` | Round `rec_mem` UP to a multiple of this. | +| `policy.cpu_headroom` | `1.5` | `rec_cpu` = ceil(ReqCPU × CPUPct/100 × this), capped at `ReqCPU`. | +| `policy.cpu_floor` | `2` | Never recommend fewer CPUs than this. | + +**The `policy` defaults are the reviewed values and changing them changes the safety properties.** +In particular `mem_round_gb` rounds **UP**, never to nearest — a safety margin that rounds down is +not a margin. A site that lowers `margin_multiplier` is choosing more OOM risk in exchange for +queue priority, and should say so to itself explicitly. + +**The `ReqCPU` cap is not optional.** Full rule: +`rec_cpu = min(ReqCPU, max(cpu_floor, ceil(ReqCPU × CPUPct/100 × cpu_headroom)))`. +Without the cap, a job at 4 CPUs and 90% utilisation computes `ceil(4 × 0.9 × 1.5) = 6` — +recommending more CPUs than were requested. The cap is present in the working system and must +survive the port. + +**`digest_user` and multi-user digests.** Merge only rows whose `User` matches `digest_user`. +Another person's peak for a same-named job would corrupt this user's recommendation with no error +anywhere. A lab-wide digest is a legitimate input; guessing whose rows to keep is not. + +Precisely what to do when other users' rows appear — **ask once, then remember**: + +- **First time:** HALT before merging anything. Report which other users are present and how many + rows each has, and ask whether this is a shared digest that should be filtered to `digest_user`. +- **On confirmation:** write `"multi_user_digest": true` into the config, then proceed, filtering + to `digest_user` and reporting the filtered-out count each week thereafter. Do not halt again. +- **If `multi_user_digest` is already `true`:** filter and report, never halt. +- **If the user declines** (this is not a shared digest to filter): merge nothing and stop. Do NOT + write `multi_user_digest`. Report the mismatch and suggest checking that `digest_user` names the + right account and that the pasted digest is the intended one. This condition re-halts on the next + run by design — nothing was resolved, so proceeding silently would be worse than asking again. +- **Never** filter silently on a first encounter, and never merge another user's rows at all. + +`digest_user` defaults to the invoking `$USER`, but the two can legitimately differ — a site where +the Slurm account name is not the login name is exactly why this is a config key and not an +inference. If no row matches `digest_user`, say so rather than merging nothing silently. + +**`digest_cluster` deliberately has no default.** Guessing it is the one error that silently +corrupts results: sizing advice from the wrong cluster's data is worse than no advice, because job +IDs are not unique across clusters. If it is unset, ask the user and write their answer to the +config — never infer it from the local hostname, which is exactly the case where the answer may +differ. + +**Determining the local cluster:** `sacctmgr -n -P list cluster format=Cluster`, falling back to +`scontrol show config | grep ClusterName`. If neither works, treat the local cluster as unknown and +say so rather than proceeding. + +**Path handling.** JSON values (`table`, `log`, `archive`) may use `~` and must be expanded before +use; create parent directories as needed when bootstrapping. + +## Bootstrap (first run, nothing exists yet) + +- **Config missing:** ask the user for `digest_cluster`, write the file with that value and the + defaults above. **Before writing defaults, check for a pre-plugin layout** — `~/.claude/slurm-sizing.md`, + `~/.claude/slurm-jobs.tsv`, `~/.claude/slurm-digests/`. Those are where an early hand-rolled + version kept its data. If any exist, offer to point the config at them instead of creating empty + files beside them. Silently starting fresh next to a populated table would strand real history. +- **Table missing:** create it with the header and the "How to read a row" prose, and zero data + rows. An empty table is valid — it means every job is unknown, and an unknown job is a reason to + measure, not to size from habit. +- **Log missing:** create it with the `#` comment header explaining the `(cluster, jobid)` join key, + then the column header `cluster jobid submitted job_name scope cwd` (tab-separated). +- **Archive dir missing:** create it. + +Never bootstrap silently — report each file created, because a user who expected existing data +needs to know the skill did not find it. From becf73fe9bafdcb4f23108cb078100a994a661e5 Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Mon, 3 Aug 2026 16:08:50 -0400 Subject: [PATCH 02/15] fix(session-tools): define the sizing table, make rec_mem monotonic, document digest provenance Whole-branch review fixes before PR. Critical: - The sizing table's schema existed nowhere; bootstrap invented one. reference/config.md now carries the literal nine-column header and the "How to read a row" prose, so bootstrap is deterministic and later merges write into known columns. - rec_mem could recompute BELOW the row's recorded peak: it was derived from this week's digest while the only stored peak was the 1-decimal peak_G the recompute forbids using. Added a full-precision peak_MB column as the authoritative running max; peak_G is now its display rounding, and rec_mem is a pure function of stored peak_MB, so it cannot decrease while peak_MB is unchanged. - Documented where a weekly digest comes from (emailed weekly to Slurm users at this institute) and that both skills are inert until the first merge. Important: - Bootstrap now runs the cluster query and offers the exact string rather than asking in free text; the log's cluster column must carry that same exact string. - One identical "determine the local cluster" procedure in all three files; hostname is dropped from the text and from allowed-tools (it names a node, not a cluster) and Bash(scontrol *) is added so the documented fallback is executable. - Replaced the unimplementable shell join (whitespace-split free-text scope, unsorted input, single key field) with one tab-delimited awk lookup on the composite (cluster, jobid) key. - Documented how the IO-BOUND flag is SET (in-process evidence only); made the CPUPct note additive and exempted IO-BOUND rows from the CPU step so the flag cannot be erased. - The digest's week-ending date is now a required argument; ask if missing, never assume today (two pastes of one digest could otherwise both pass the duplicate check). - Added a decline branch: "enabled": false silences both skills permanently. - Deleted an unreachable zero-match escape hatch that contradicted the line above it. - README: invocation namespace is the plugin, /session-tools:, not /whitehead:. Minor: sample config uses placeholders, not real values; supporting files cited as markdown links; (M) documented as a unit annotation, not required header text; dropped unused Bash(cat *)/Bash(ls *); reworded the circular "config may relocate itself" line. --- README.md | 33 ++- .../skills/slurm-digest/SKILL.md | 254 ++++++++++++------ .../skills/slurm-sizing/SKILL.md | 94 +++++-- .../skills/slurm-sizing/reference/config.md | 232 +++++++++++++--- 4 files changed, 465 insertions(+), 148 deletions(-) diff --git a/README.md b/README.md index 191c1aa..bbec7f7 100644 --- a/README.md +++ b/README.md @@ -8,16 +8,31 @@ A shared plugin marketplace for [Claude Code](https://code.claude.com/) at White |--------|-------|--------------| | [`fry-python-tool`](plugins/fry-python-tool/) | `fry-python-tool` | Find existing GPU-powered computational biology tools on the Whitehead fry cluster, or scaffold a new one. For single-purpose Python tools that produce outputs (embeddings, segmentations, predictions) for downstream analysis. | | [`session-tools`](plugins/session-tools/) | `self-assess` | Audit the current Claude Code session for errors, inefficiencies, and repeated workarounds, then produce actionable recommendations to improve skills, scripts, CLAUDE.md, and tooling. | -| [`session-tools`](plugins/session-tools/) | `slurm-sizing` | Loads automatically before and after Slurm job submission. Sizes `--mem`/`--cpus-per-task` from a table of measured past usage instead of habit, and logs what each new job ran so a later `slurm-digest` merge can attach real scope to it. | -| [`session-tools`](plugins/session-tools/) | `slurm-digest` | User-invoked. Merges a weekly Slurm usage digest into the measured sizing table `slurm-sizing` reads from. | +| [`session-tools`](plugins/session-tools/) | `slurm-sizing` | Written to load on its own before and after Slurm job submission (via its skill description — Claude's discretion, not a hook). Sizes `--mem`/`--cpus-per-task` from a table of measured past usage instead of habit, and logs what each new job ran so a later `slurm-digest` merge can attach real scope to it. | +| [`session-tools`](plugins/session-tools/) | `slurm-digest` | User-invoked as `/session-tools:slurm-digest`. Merges the weekly Slurm usage digest email into the measured sizing table `slurm-sizing` reads from. | -After installing (see below), invoke a skill with `/whitehead:` — e.g., `/whitehead:self-assess`. +After installing (see below), invoke a skill with `/:` — the namespace is +the **plugin** name, not the marketplace name. E.g. `/session-tools:self-assess` and +`/session-tools:slurm-digest` are both in the `session-tools` plugin. + +**Where the weekly Slurm digest comes from.** `slurm-digest` does not generate a digest — it merges +one you are already sent. Whitehead emails a Slurm usage digest **once a week** to users who ran +jobs, listing each job's requested versus used memory and CPU. You paste that table into +`/session-tools:slurm-digest`, along with the week-ending date it covers, and it folds the numbers +into your sizing table. `slurm-sizing`'s data — the usage table and job log it reads and writes — lives under your own -`~/.claude/slurm-sizing/`. It is **not shipped with the plugin** and starts empty; it becomes -useful once you've merged your first digest with `slurm-digest`. A recommendation is only -actionable when the workload scope behind the measurement is known: rows with an unknown scope -are marked as lower bounds and can justify *raising* a request, but never *lowering* one. +`~/.claude/slurm-sizing/`. It is **not shipped with the plugin** and starts empty, so **both skills +are inert until you merge your first digest**: every job is "unknown", which `slurm-sizing` reports +as a reason to measure rather than licence to guess. A recommendation is only actionable when the +workload scope behind the measurement is known: rows with an unknown scope are marked as lower +bounds and can justify *raising* a request, but never *lowering* one. + +**Note for existing `session-tools` users.** Version 1.1.0 adds `slurm-sizing`, which is designed +to load **on its own** — before and after anything that submits a cluster job — rather than only +when you invoke it. If you installed `session-tools` for `self-assess`, updating changes that +behaviour. If it does not apply to you, say so the first time it asks: it writes +`{"enabled": false}` to `~/.claude/slurm-sizing/config.json` and then stays silent permanently. ## Using This Marketplace @@ -45,7 +60,9 @@ Install a plugin: /plugin install plugin-name@whitehead ``` -Installed plugins are available immediately. Plugin skills show up as `/whitehead:skill-name` commands. +Installed plugins are available immediately. Plugin skills show up as `/:` +commands — the namespace is the plugin's name, not the marketplace's. For example, the +`self-assess` skill in the `session-tools` plugin is `/session-tools:self-assess`. ### Update plugins diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index e72296b..c7799b5 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -2,38 +2,71 @@ name: slurm-digest description: >- Merge a weekly Slurm usage digest into the sizing table used by slurm-sizing. User-invoked via - /session-tools:slurm-digest — paste the week's usage digest as the argument and run it - explicitly; this skill does not trigger on its own the way slurm-sizing does. + /session-tools:slurm-digest — paste the week's usage digest as the argument AND give the + week-ending date it covers, then run it explicitly; this skill does not trigger on its own the + way slurm-sizing does. user-invocable: true -allowed-tools: Read, Write, Edit, Bash(cat *), Bash(ls *), Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(join *), Bash(sacct *), Bash(sacctmgr *), Bash(hostname) +allowed-tools: Read, Write, Edit, Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(sacct *), Bash(sacctmgr *), Bash(scontrol *) --- # Slurm Digest Merge -Full config contract (keys, defaults, bootstrap file formats, and the multi-user filtering rules -referenced in Step 3 below): `../slurm-sizing/reference/config.md`. Read `digest_cluster`, -`digest_user`, `table`, `log`, `archive`, `multi_user_digest`, and `policy` from -`~/.claude/slurm-sizing/config.json` before anything else below; if that config doesn't exist -yet, follow its "Bootstrap" section first rather than guessing any of these values. +## Where this digest comes from + +This skill does **not** produce a digest; it merges one you already have. At Whitehead, users who +ran Slurm jobs are sent a usage digest **by email, once a week** — a table of each job's requested +versus used memory and CPU. That email is the input: paste its table below and give the +week-ending date it covers. + +Consequently the whole system is **inert until the first digest is merged**. A fresh install has an +empty sizing table, so `slurm-sizing` reports every job as unknown — which is a reason to measure, +not licence to guess. There is no substitute input; do not invent an `sacct` recipe to synthesise +one (`sacct` is used later only to *annotate* rows this digest already contains). + +## Config + +Full config contract — keys, defaults, the table and log file formats, the shared "determine the +local cluster" procedure, and the multi-user filtering rules referenced in Step 3 below — is in +[../slurm-sizing/reference/config.md](../slurm-sizing/reference/config.md). Read `enabled`, +`digest_cluster`, `digest_user`, `table`, `log`, `archive`, `multi_user_digest`, and `policy` from +`~/.claude/slurm-sizing/config.json` before anything else below; if that config doesn't exist yet, +follow its "Bootstrap" section first rather than guessing any of these values. + +**If `enabled` is `false`**, the user has declined this system. Merge nothing. Report that it is +turned off and offer to re-enable it (set `"enabled": true` and run bootstrap); proceed only if +they confirm. Merge the Slurm digest pasted below into the sizing table at the configured `table` path -(default `~/.claude/slurm-sizing/table.md`). +(default `~/.claude/slurm-sizing/table.md`). The table's nine columns are `job_name`, `peak_MB`, +`peak_G`, `n`, `scope`, `last_seen`, `rec_mem`, `rec_cpu`, `notes` — see +[../slurm-sizing/reference/config.md](../slurm-sizing/reference/config.md) → "Table file format" +for what each one means and the exact header to write if the table has to be created. ## Procedure -1. **Refuse duplicates (precondition — check this FIRST, before parsing or merging anything).** - Determine the digest's date and check whether `/YYYY-MM-DD.tsv` already exists for it, - where `` is the configured `archive` path (default `~/.claude/slurm-sizing/digests`). - **If it does, STOP here: change nothing** — do not parse, join, merge, or write anything — +1. **Establish the week-ending date, then refuse duplicates (precondition — check this FIRST, + before parsing or merging anything).** The digest's date is **a required argument**, supplied + alongside the pasted table (e.g. `/session-tools:slurm-digest 2026-08-01 `). The + digest body itself contains no date column, so it cannot be recovered from the paste. + **If the date is missing, ASK for it — never assume today's date.** Defaulting to today lets the + same digest pasted on two different days archive under two filenames, pass this duplicate check + both times, and double `n` — exactly the corruption this step exists to prevent. + With the date in hand, check whether `/YYYY-MM-DD.tsv` already exists for it (`Read` + it; a "no such file" error is the not-archived case), where `` is the configured + `archive` path (default `~/.claude/slurm-sizing/digests`). + **If it exists, STOP here: change nothing** — do not parse, join, merge, or write anything — report that the digest is already archived for that date, and ask whether to force. Re-merging an already-counted digest would inflate `n` (nine `score` runs would read as eighteen), and `n` is the signal that says whether a recommendation is trustworthy. Only proceed past this step if no file exists for that date, or the user has explicitly instructed you to force a re-merge. -2. **Parse.** Read the pasted table. Columns are `User JobID JobName ReqMem(M) UsedMem(M) - MemPct ReqCPU UsedCPU CPUPct Elapsed`. Skip the `OVERALL` line, skip `bash`, `python3`, and - any bare shell or interpreter name. +2. **Parse.** Read the pasted table. Columns are, in order: `User`, `JobID`, `JobName`, `ReqMem`, + `UsedMem`, `MemPct`, `ReqCPU`, `UsedCPU`, `CPUPct`, `Elapsed`. Memory columns are in + **megabytes** — written `ReqMem(M)` / `UsedMem(M)` elsewhere in this file to state the unit, + which is **not** part of the header text; do not require the literal `(M)` to be present, and do + not reject a digest over it. Skip the `OVERALL` line, skip `bash`, `python3`, and any bare shell + or interpreter name. 3. **Filter to `digest_user` — a consent-gated filtering procedure, not part of parsing.** Merge only rows whose `User` matches the configured `digest_user` (default: the invoking @@ -60,45 +93,65 @@ Merge the Slurm digest pasted below into the sizing table at the configured `tab 4. **Exclude capped runs.** Drop any row with `MemPct >= 99`. Its MaxRSS is a ceiling imposed by `--mem`, not a measured peak. Note in the report how many rows were dropped this way. -5. **Join for scope, on `(cluster, jobid)` — never `jobid` alone.** The log at the configured - `log` path (default `~/.claude/slurm-sizing/jobs.tsv`) begins with `#`-prefixed comment lines - before its header row — **filter them first**: run `grep -v '^#' ` before running - `awk`/`join` on that file. **This join is carried out with real shell commands, not by - reasoning over `Read` output** — the failure mode below is what `join` itself does, and only - manifests when it is actually invoked: treating the comments as data feeds `join` unsorted - input (it will warn "is not sorted") and can produce false non-matches. The filtered log's - header is - `cluster jobid submitted job_name scope cwd`. +5. **Look up scope in the log, keyed on `(cluster, jobid)` — never `jobid` alone.** The log at the + configured `log` path (default `~/.claude/slurm-sizing/jobs.tsv`) is tab-separated and begins + with `#`-prefixed comment lines before its header row + `cluster jobid submitted job_name scope cwd`. **Digests come from the configured `digest_cluster` only; the log's rows carry their own - `cluster` value, which may name a different cluster (e.g. `cluster-b`) for jobs submitted - elsewhere.** `digest_cluster` and other clusters are separate Slurm ID namespaces — the same - `jobid` number can label unrelated jobs on each one — so a bare `jobid` join is a correctness - bug: it will silently splice an unrelated cluster's job onto this digest's scope and produce a - confidently wrong recommendation. Joining on `jobid` alone is never acceptable, even as a - shortcut. Before matching, **exclude every log row whose `cluster` is not the digest's own - `digest_cluster`** — those rows are not eligible matches regardless of `jobid` equality. For - each remaining digest row, look up `JobID` among the rows that survived that exclusion. A - match supplies the workload scope. No match means `scope: unknown`. **If zero rows match - against `digest_cluster` log rows and `digest_cluster` rows exist in the log, say so - prominently** — a silently failing join makes every recommendation a lower bound and the - system quietly stops working. (A zero-match result is *expected*, not a defect, when the - filtered log contains no `digest_cluster` rows at all — e.g. it currently holds only rows - whose provenance is some other cluster.) + `cluster` value, which may name a different cluster for jobs submitted elsewhere.** + `digest_cluster` and other clusters are separate Slurm ID namespaces — the same `jobid` number + can label unrelated jobs on each one — so a bare `jobid` lookup is a correctness bug: it will + silently splice an unrelated cluster's job onto this digest's scope and produce a confidently + wrong recommendation. Joining on `jobid` alone is never acceptable, even as a shortcut. + + **Do this with a real shell command, not by reasoning over `Read` output**, so the composite key + and the tab delimiter are actually enforced. One `awk` lookup does all of it — comment + stripping, the `cluster` equality test, and the `jobid` membership test — in a single pass: + + ```bash + grep -v '^#' "$LOG" | awk -F'\t' -v c="$DIGEST_CLUSTER" -v ids="$JOBIDS" ' + BEGIN { n = split(ids, a, ","); for (i = 1; i <= n; i++) want[a[i]] = 1 } + $1 == c && ($2 in want) { print $2 "\t" $5 } + ' + ``` + + where `$JOBIDS` is the comma-separated list of `JobID`s from the surviving digest rows. Each + output line is `jobidscope` for a row that matched **both** halves of the key. + + Three properties are load-bearing and must not be dropped: + - **`-F'\t'`** — the file is tab-separated and `scope` is free text containing spaces. Default + whitespace splitting shatters `scope` across fields and corrupts the result. + - **`$1 == c`** — log rows from any other cluster are excluded before `jobid` is even consulted. + This is the composite key; `awk` is used here precisely because `join` takes a single key + field and needs both inputs pre-sorted, while this log is append-ordered. + - **`grep -v '^#'`** — the comment header is not data. + + A digest row with no output line gets `scope: unknown`. **If zero rows match, say so + prominently** — a silently failing lookup makes every recommendation a lower bound and the + system quietly stops working. Because `slurm-sizing` refuses to log on any cluster other than + `digest_cluster`, a populated log should contain `digest_cluster` rows; a zero match against a + non-empty log means the lookup is broken, not that there was nothing to find. 6. **Enrich unmatched rows from `sacct` — a conditional recovery procedure, not part of the - join.** For every digest row that did NOT match a submission-log entry in Step 5, attempt to + lookup.** For every digest row that did NOT match a submission-log entry in Step 5, attempt to recover a *derived* scope directly from Slurm before falling back to `scope: unknown`. The six bullets below are sequential and order-dependent — cluster guard, then batch query, then discard step rows, then collapse array rows, then the miss guard, then record — treat them as - an ordered procedure to follow in full, not as loose elaboration on the join. + an ordered procedure to follow in full, not as loose elaboration on the lookup. - **Cluster guard, checked first.** `sacct` only sees the LOCAL cluster's accounting database — **verified in practice**: a job ID from the digest's cluster does not resolve via `sacct` run against a different cluster's accounting database (`sacct -L -j ` returns nothing for it there), and `sacctmgr -n -P list cluster format=Cluster` lists only - the local cluster's name. Determine the local cluster (e.g. `sacctmgr -n -P list cluster - format=Cluster` or `hostname`) and compare it to the digest's cluster (`digest_cluster` from - config). If they differ, **skip enrichment entirely** for the whole digest — do not query - `sacct` — and say so plainly in the report (Step 12). Never fabricate a scope to compensate. + the local cluster's name. Determine the local cluster with the **shared procedure** in + [../slurm-sizing/reference/config.md](../slurm-sizing/reference/config.md) → "Determining the + local cluster" (`sacctmgr -n -P list cluster format=Cluster`, falling back to + `scontrol show config | grep ClusterName`, taking the value after the `=`) — **never + `hostname`**, which returns a node name, not a cluster name, and would therefore fail this + comparison on essentially every site while reporting the result as a deliberate skip. Compare + it to the digest's cluster (`digest_cluster` from config). If they differ, **skip enrichment + entirely** for the whole digest — do not query `sacct` — and say so plainly in the report + (Step 12). If the local cluster is undeterminable, treat that the same way: skip and say so. + Never fabricate a scope to compensate. - **Batch query, one field per call — never a combined multi-field query.** Query once for every unmatched row, not per row, but query `SubmitLine` and `WorkDir` in **separate** calls: `sacct -j --format=JobID,SubmitLine --parsable2 --noheader` and, only @@ -139,42 +192,64 @@ Merge the Slurm digest pasted below into the sizing table at the configured `tab Write it into the row's `scope` column as `derived: ` — e.g. `derived: sbatch --dependency=afterok:7932155 --array=0-11%4 --mem=32G scripts/08_screen_zarr.sh` — visibly distinct from a declared scope (no prefix) that came - from the join. **A `derived:` scope does NOT clear the `>=` prefix on `rec_mem`** — a submit + from the lookup. **A `derived:` scope does NOT clear the `>=` prefix on `rec_mem`** — a submit line records what the measurement WAS, not that the next run shares that scope — so Step 8's asymmetric scope rule still treats the row as unknown-scope for sizing purposes until a human promotes it (Step 10). -7. **Merge.** For each job name: - `peak_G = max(existing_peak_G, this week's max UsedMem_MB / 1024)`. - **The `/1024` is not optional.** `existing_peak_G` (the table's `peak_G` column) is already in - GB; `UsedMem` in the digest is in MB (`UsedMem(M)`, per Step 2's column header) — skip the - conversion and you corrupt `peak_G` by roughly 1000x (e.g. the first merge of a 4794.71 MB - reading would write `peak_G = 4794` where `4.7` belongs). Add to `n`, update `last_seen`. - Never lower a peak. +7. **Merge the peak.** For each job name, update the two memory columns: + ``` + peak_MB = max(existing peak_MB, this week's max UsedMem_MB) # full precision, MB + peak_G = round(peak_MB / 1024, 1) # display only + ``` + `peak_MB` is the **authoritative, full-precision** peak in megabytes and is what every later + step computes from; `peak_G` is a display rounding of it and is never an input to anything. + **The `/1024` is not optional, and it belongs only in the `peak_G` line.** `UsedMem` in the + digest is in MB, so the `max` is MB-vs-MB with no conversion; dividing there, or *failing* to + divide when writing `peak_G`, corrupts the column by roughly 1000x (a 4794.71 MB reading would + display `peak_G = 4794` where `4.7` belongs). Add to `n`, set `last_seen` to the digest's + week-ending date. **Never lower `peak_MB`** — it is a running maximum across all weeks ever + merged, so a week in which the job ran a smaller input leaves it untouched. + If a row predates the `peak_MB` column, seed it as `peak_G * 1024` (lossy but never lower), + say so in the report, and treat its `rec_mem` as a lower bound until the next digest. **Skip rows flagged `IO-BOUND`.** That flag lives in the `notes` column of the job's **existing row in the sizing table** (the configured `table` path) — never in the incoming digest, which carries no such marker. Check the current table row before merging, not the pasted digest; a digest row for an IO-BOUND job looks like any other row and its MaxRSS is - page cache, not demand, so it must not enter the max. - -8. **Recompute `rec_mem`.** `rec_mem = policy.margin_multiplier x full-precision peak` (default - `margin_multiplier = 2`), where full-precision peak is `max(UsedMem_MB) / 1024` — **not** the - 1-decimal `peak_G` value displayed in the table. Floor the result at `policy.mem_floor_gb` - (default 8 G), then round UP to the next multiple of `policy.mem_round_gb` (default 4 G) — - never down; this is a safety margin, so a peak that lands exactly on a multiple of - `mem_round_gb` stays there, and any remainder pushes to the next multiple up. Worked example at - the default values: a full-precision peak of 13.2 G gives `2 x 13.2 = 26.4`, which rounds up to - 28 G, not down to 24 G. **These are the reviewed defaults, not hardcoded law** — a site that - lowers `margin_multiplier`, or any other `policy` value, in config is choosing more OOM risk in - exchange for queue priority, and should make that choice deliberately, in config, not by - editing this file. Then apply the asymmetric scope rule: + page cache, not demand, so it must not enter the max. An `IO-BOUND` row is left **entirely + untouched** by Steps 7, 8 and 9 — peak, recommendations and notes alike. + +8. **Recompute `rec_mem` from the stored `peak_MB`.** + `rec_mem = policy.margin_multiplier x (peak_MB / 1024)` (default `margin_multiplier = 2`), + where `peak_MB` is **the row's stored full-precision peak after Step 7** — **not** this week's + digest rows, and **not** the 1-decimal `peak_G` displayed in the table. Floor the result at + `policy.mem_floor_gb` (default 8 G), then round UP to the next multiple of `policy.mem_round_gb` + (default 4 G) — never down; this is a safety margin, so a peak that lands exactly on a multiple + of `mem_round_gb` stays there, and any remainder pushes to the next multiple up. Worked example + at the default values: a stored peak of 13.2 G gives `2 x 13.2 = 26.4`, which rounds up to + 28 G, not down to 24 G. + + **`rec_mem` must never decrease while `peak_MB` is unchanged.** Because it is a pure function of + the stored `peak_MB` and the `policy` values, this holds automatically — but it is the property + to check if the arithmetic is ever changed. Computing it from *this week's* peak instead would + break it: a job that peaked at 14963.82 MB (`peak_G 14.6`, `rec_mem 32G`) and then ran a smaller + input peaking at 5000 MB would keep `peak_G 14.6` but drop to `rec_mem 12G` — a recommendation + below the row's own recorded peak, and an OOM on the next full-size run. + + **These are the reviewed defaults, not hardcoded law** — a site that lowers `margin_multiplier`, + or any other `policy` value, in config is choosing more OOM risk in exchange for queue priority, + and should make that choice deliberately, in config, not by editing this file. + + Then apply the asymmetric scope rule, comparing this week's `scope` (Step 5/6) against the one + recorded in the row's `scope` column: - scope matches the recorded scope -> plain recommendation, no prefix - scope is larger than recorded -> do NOT lower; scale by the known ratio or keep the prior request and treat this run as a fresh measurement - scope is smaller than recorded -> a smaller scope is not evidence about the larger one; do NOT lower anything, treat it like unknown scope for recommendation purposes, and note the scope it was actually measured at - - scope unknown -> prefix `>=`; the row may justify raising a request, never lowering one + - scope unknown (empty, or a `derived:` value not yet confirmed by a human) -> prefix `>=`; the + row may justify raising a request, never lowering one 9. **CPU.** One formula across the whole range — no bands, no carve-outs: `rec_cpu = min(ReqCPU, max(policy.cpu_floor, ceil(ReqCPU x CPUPct / 100 x @@ -185,7 +260,15 @@ Merge the Slurm digest pasted below into the sizing table at the configured `tab so the cap brings it back down to 4. The `cpu_headroom` multiplier (default 1.5x) is deliberate headroom over the observed average, not a serial/parallel classification — e.g. a job at 25% of 16 requested CPUs used ~4 cores on average, so `ceil(16 x 0.25 x 1.5) = 6`. - Record the observed `CPUPct` in `notes`. + + Record the observed `CPUPct` in `notes` **additively**: `notes` is one shared free-text column + that also holds the load-bearing `IO-BOUND` flag and any human annotation. Update or append the + `CPUPct` fragment while **preserving every other fragment already in the cell** — never + overwrite the column with a bare `CPUPct `. Use a separated form such as + `IO-BOUND (getrusage peak 3.2G); CPUPct 45` so fragments stay individually editable. + **`IO-BOUND` rows are exempt from this step**, as from Steps 7 and 8: do not recompute their + `rec_cpu` and do not touch their `notes`. An overwritten note erases the flag, and the next + week's page-cache MaxRSS then enters the peak — the exact failure the flag exists to prevent. 10. **Prompt for scope.** List unmatched job names ranked by wasted reservation `(ReqMem - rec_mem) x n`, largest first, capped at 10. Ask the user to supply scope for those. @@ -201,19 +284,22 @@ Merge the Slurm digest pasted below into the sizing table at the configured `tab prompted exactly as before. 11. **Persist, then archive.** This is the point the on-disk table changes: write the fully - merged and recomputed table (Steps 7-10 applied) back to the configured `table` path now. - Only after that write succeeds, write the raw pasted digest to `/YYYY-MM-DD.tsv` - (the configured `archive` path) — the duplicate check already happened in Step 1, so this - write should never collide with an existing file under normal operation. - -12. **Report.** State: rows parsed, rows dropped as capped, join matches, job names updated, - job names new, peaks that rose, and the total reservation delta if the recommendations were - applied. Be explicit that `>=` rows are not actionable for sizing down. + merged and recomputed table (Steps 7-10 applied) back to the configured `table` path now, + preserving the nine-column header. Only after that write succeeds, write the raw pasted digest + to `/YYYY-MM-DD.tsv` under the week-ending date from Step 1 (creating the configured + `archive` directory with `mkdir -p` if it does not exist) — the duplicate check already + happened in Step 1, so this write should never collide with an existing file under normal + operation. + +12. **Report.** State: the week-ending date merged, rows parsed, rows dropped as capped, scope + matches from the log, job names updated, job names new, peaks that rose, and the total + reservation delta if the recommendations were applied. Be explicit that `>=` rows are not + actionable for sizing down. Also state: how many unmatched rows were enriched with a `derived:` scope from `sacct`, how many attempted enrichments missed (empty `SubmitLine` / retention gap), and whether - enrichment was skipped entirely because the digest's cluster differs from the local cluster - (the Step 6 cluster guard) — if skipped, say so explicitly rather than silently omitting the - counts. + enrichment was skipped entirely because the digest's cluster differs from the local cluster, + or because the local cluster was undeterminable (the Step 6 cluster guard) — if skipped, say + so explicitly rather than silently omitting the counts. Also state: how many rows were filtered out because their `User` did not match `digest_user` (Step 3), whenever that filtering applied. @@ -221,10 +307,14 @@ Merge the Slurm digest pasted below into the sizing table at the configured `tab - Advisory only. Do not edit job scripts, launcher scripts, or any pipeline code. Do not submit, cancel, or modify jobs. -- Never lower a peak; never remove an `IO-BOUND` flag without in-process evidence - (`psutil`-style RSS sampling), because `sacct` MaxRSS cannot distinguish demand from page cache. -- If the digest's column layout differs from the above, stop and report rather than guessing. +- Never lower `peak_MB`; never remove an `IO-BOUND` flag without in-process evidence + (`getrusage`/`psutil`-style RSS sampling), because `sacct` MaxRSS cannot distinguish demand from + page cache. +- If the digest's column layout differs from Step 2's order, stop and report rather than guessing. + A missing `(M)` unit annotation in the header text is **not** a layout difference. ## Digest +Week-ending date and pasted digest: + $ARGUMENTS diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index 2bd3139..5271170 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -9,27 +9,46 @@ description: >- anything whose output includes "Submitted batch job"); and any choice of `--mem`/`--cpus-per-task`/`--time`. Also triggers on "run this on the cluster", "how much memory should this job get", "why did my job OOM", "am I over-requesting", or any question - about Slurm resource sizing. SKIP only when no cluster job is involved at any remove. + about Slurm resource sizing. SKIP only when no cluster job is involved at any remove, or when + the user has turned this system off (`"enabled": false` in its config). user-invocable: true -allowed-tools: Read, Write, Edit, Bash(cat *), Bash(ls *), Bash(mkdir *), Bash(sacctmgr *), Bash(scontrol *), Bash(grep *) +allowed-tools: Read, Write, Edit, Bash(mkdir *), Bash(sacctmgr *), Bash(scontrol *), Bash(grep *) --- # Slurm Job Sizing -Full config contract (keys, defaults, bootstrap file formats): `reference/config.md`. Consult it -whenever any of the files below don't exist yet. +Full config contract — keys, defaults, the table and log file formats, and the shared +"determine the local cluster" procedure — is in +[reference/config.md](reference/config.md). Consult it whenever any of the files below +don't exist yet, and whenever you need the exact column layout of the table or the log. + +The table this skill reads is filled in by merging a weekly Slurm usage digest with +`/session-tools:slurm-digest`. Where that digest comes from, and the fact that this skill is inert +until the first one is merged, are documented in +[reference/config.md](reference/config.md) → "Where the weekly digest comes from". + +## 0. Off switch — check this before the cluster gate + +Read `~/.claude/slurm-sizing/config.json` (this path is fixed; only the `table`/`log`/`archive` +data paths inside it are configurable). + +- **`"enabled": false`** — the user has declined this system. Do nothing: do not consult the + table, do not log, and do **not** prompt or offer to re-enable. Stay silent. +- **Config file does not exist:** do not guess anything. Go to **Bootstrap** (§6) first. +- Otherwise (`enabled` absent or `true`): continue to §1. ## 1. Cluster gate — check this before anything else -1. Read `digest_cluster` from `~/.claude/slurm-sizing/config.json` (path may be overridden in the - config itself). - - **Config file does not exist:** do not guess anything. Go to **Bootstrap** (§6) first. -2. Determine the local cluster: `sacctmgr -n -P list cluster format=Cluster`, falling back to - `scontrol show config | grep ClusterName`. +1. Read `digest_cluster` from the config. +2. Determine the local cluster using the shared procedure in + [reference/config.md](reference/config.md) → "Determining the local cluster": + `sacctmgr -n -P list cluster format=Cluster`, falling back to + `scontrol show config | grep ClusterName` (take the value after the `=`, trimmed). **Never use + `hostname`** — it names a node, not a cluster. - **Both commands fail or return nothing:** the local cluster is undeterminable. Say so explicitly and STOP — do not consult the table, do not append to the log. "Unknown" is never treated as a match. -3. Compare the two values. +3. Compare the two values — an exact string match. - **They match:** proceed to §2. - **They differ:** say so explicitly (name both clusters) and STOP. Do not consult the table and do not append to the log. This system is inert on any cluster other than the one the @@ -38,11 +57,15 @@ whenever any of the files below don't exist yet. ## 2. Before sizing any job — read the table first -Read the table at the configured `table` path (default `~/.claude/slurm-sizing/table.md`). +Read the table at the configured `table` path (default `~/.claude/slurm-sizing/table.md`). Its +nine columns (`job_name`, `peak_MB`, `peak_G`, `n`, `scope`, `last_seen`, `rec_mem`, `rec_cpu`, +`notes`) and how to read each one are specified in +[reference/config.md](reference/config.md) → "Table file format". - **Table file does not exist:** go to **Bootstrap** (§6). - **Job name is present in the table:** use that row's `rec_mem` / `rec_cpu` and state which row - you used (e.g. "using the `align_star` row: rec_mem=32G, rec_cpu=8"). + you used (e.g. "using the `align_star` row: rec_mem=32G, rec_cpu=8"). Quote `n` too — `n = 1` is + a single observation, not a characterisation. - **The value carries a `>=` prefix:** that's a lower bound from a run whose scope was never recorded, so the true peak could be higher. It may justify *raising* the request above the table value; it must never justify *lowering* the request below it. @@ -54,9 +77,21 @@ Read the table at the configured `table` path (default `~/.claude/slurm-sizing/t `MaxRSS` on a job that reads or writes a lot of data includes kernel page cache charged to that job's cgroup — it is not memory the job actually demanded, and sizing from it can overstate the -real requirement by a large multiple. Rows marked `IO-BOUND` in the table are hand-pinned from -in-process measurement (e.g. `resource.getrusage`, a memory profiler) instead — trust those over -any `sacct`-derived figure for the same job, and don't recompute an `IO-BOUND` row from `sacct`. +real requirement by a large multiple. Rows whose `notes` column contains `IO-BOUND` are hand-pinned +from in-process measurement instead — trust those over any `sacct`-derived figure for the same job, +and don't recompute an `IO-BOUND` row from `sacct`. + +**Setting the flag** (this is how a row *becomes* `IO-BOUND` — nothing else sets it): + +1. Get **in-process** evidence of the real peak RSS — `resource.getrusage(...).ru_maxrss` inside + the job, a `psutil`-style RSS sampler, or a memory profiler. A digest or `sacct` number can + never justify the flag: distrusting those numbers is the entire point of it. +2. Set the row's `peak_MB` to that measured peak; `peak_G` and `rec_mem` follow from it by the + formulas in [reference/config.md](reference/config.md). +3. **Append** `IO-BOUND` to `notes`, preserving whatever text is already there, and record where + the measurement came from — e.g. `IO-BOUND (getrusage peak 3.2G, 2026-08-03); CPUPct 45`. + +Removing the flag requires the same class of evidence that set it — never a digest, never `sacct`. ## 4. After submitting — log what it actually ran @@ -67,6 +102,8 @@ cluster jobid submitted job_name scope cwd ``` - **Log file does not exist:** go to **Bootstrap** (§6), then append the row. +- `cluster` must be the exact `digest_cluster` string from the config — the digest merge filters + log rows by exact match on this column, so a variant spelling makes the row invisible. - `scope` is a short, concrete description of the workload this run did — e.g. "12 tasks x 1565 tiles (FULL)", "5 of 100 perturbations" — not a repeat of the job name. - A peak logged with no `scope` is only ever a lower bound: once the job has finished, `sacct` has @@ -82,24 +119,29 @@ attaching to another cluster's job. ## 6. Bootstrap — config, table, or log missing -Full file formats and rationale: `reference/config.md` → "Bootstrap" section. The branches, so -none gets skipped: +Full file formats, the literal table header, and the rationale are in +[reference/config.md](reference/config.md) → "Bootstrap". The branches, so none gets skipped: -- **Config missing:** ask the user for `digest_cluster` — there is no safe default (§1). Before - writing a fresh file, check for a pre-plugin layout (`~/.claude/slurm-sizing.md`, +- **Config missing:** check first for a pre-plugin layout (`~/.claude/slurm-sizing.md`, `~/.claude/slurm-jobs.tsv`, `~/.claude/slurm-digests/`); if any exist, offer to point the new - config at them instead of starting empty files beside real history. -- **Table missing:** create it with the header and explanatory prose, zero data rows. An empty - table is a valid, complete state — every job is unknown, which is a reason to measure, not a - reason to size from habit. + config at them instead of starting empty files beside real history. Then establish + `digest_cluster` — **do not ask for it in free text.** Run the §1 query, show the user the exact + string it returned, and offer it as the answer, along with the option to name a different cluster + and the option to decline ("not applicable / I have no weekly digest"). Declining writes + `{"enabled": false}`, creates nothing else, and this skill then stays silent permanently (§0). +- **Table missing:** create it with the nine-column header row and the "How to read a row" prose + given verbatim in [reference/config.md](reference/config.md) → "Table file format", and zero data + rows. An empty table is a valid, complete state — every job is unknown, which is a reason to + measure, not a reason to size from habit. - **Log missing:** create it with the `(cluster, jobid)` join-key comment header, then the tab-separated column header `cluster jobid submitted job_name scope cwd`. -- **Archive dir missing:** create it (empty). +- **Archive dir missing:** create it (empty) with `mkdir -p`. Report every file created during bootstrap — a user who expected existing data needs to know the skill didn't find it, rather than silently starting fresh next to it. ## 7. Weekly maintenance -This table is only as good as the digests merged into it. Use `/session-tools:slurm-digest` to -merge a new weekly usage digest into the table. +This table is only as good as the digests merged into it. When the week's Slurm usage digest email +arrives, merge it with `/session-tools:slurm-digest`, passing the week-ending date along with the +pasted digest. diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index be07c01..edbac8b 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -5,8 +5,9 @@ read it. If it does not exist, the skills bootstrap it (below). ```json { - "digest_cluster": "fry", - "digest_user": "alice", + "enabled": true, + "digest_cluster": "", + "digest_user": "", "table": "~/.claude/slurm-sizing/table.md", "log": "~/.claude/slurm-sizing/jobs.tsv", "archive": "~/.claude/slurm-sizing/digests", @@ -21,13 +22,18 @@ read it. If it does not exist, the skills bootstrap it (below). } ``` +The two `<...>` values above are placeholders — fill them in with your own. Do not copy them +verbatim, and in particular do not copy a cluster name from anyone else's config: a wrong +`digest_cluster` is the one setting that corrupts every recommendation silently (below). + | Key | Default | Meaning | |---|---|---| -| `digest_cluster` | **none — must be set** | The Slurm cluster whose weekly usage digest feeds this system. Compared against the local cluster before anything is consulted or logged. | +| `enabled` | `true` | Master switch. When `false`, both skills stay silent and do nothing (see "Declining"). | +| `digest_cluster` | **none — must be set** | The Slurm cluster whose weekly usage digest feeds this system. Compared against the local cluster before anything is consulted or logged. Must be the cluster's exact Slurm name (below). | | `digest_user` | the invoking `$USER` | Which user's rows to merge. The digest carries a `User` column and may contain several people's jobs. | | `table` | `~/.claude/slurm-sizing/table.md` | The sizing table. | | `log` | `~/.claude/slurm-sizing/jobs.tsv` | The submission log where scope is recorded. | -| `archive` | `~/.claude/slurm-sizing/digests` | Directory of raw archived digests, one per date. | +| `archive` | `~/.claude/slurm-sizing/digests` | Directory of raw archived digests, one per week-ending date. | | `multi_user_digest` | `false` | This digest is known to contain other users' rows and should be filtered without halting. | | `policy.margin_multiplier` | `2` | `rec_mem` = this × peak. | | `policy.mem_floor_gb` | `8` | Never recommend less than this. | @@ -35,10 +41,11 @@ read it. If it does not exist, the skills bootstrap it (below). | `policy.cpu_headroom` | `1.5` | `rec_cpu` = ceil(ReqCPU × CPUPct/100 × this), capped at `ReqCPU`. | | `policy.cpu_floor` | `2` | Never recommend fewer CPUs than this. | -**The `policy` defaults are the reviewed values and changing them changes the safety properties.** -In particular `mem_round_gb` rounds **UP**, never to nearest — a safety margin that rounds down is -not a margin. A site that lowers `margin_multiplier` is choosing more OOM risk in exchange for -queue priority, and should say so to itself explicitly. +**The `policy` values are tunable defaults, not invariants — but they are the reviewed defaults, +and changing them changes the safety properties.** In particular `mem_round_gb` rounds **UP**, +never to nearest — a safety margin that rounds down is not a margin. A site that lowers +`margin_multiplier` is choosing more OOM risk in exchange for queue priority, and should say so to +itself explicitly, in config, rather than by editing the skills. **The `ReqCPU` cap is not optional.** Full rule: `rec_cpu = min(ReqCPU, max(cpu_floor, ceil(ReqCPU × CPUPct/100 × cpu_headroom)))`. @@ -46,9 +53,56 @@ Without the cap, a job at 4 CPUs and 90% utilisation computes `ceil(4 × 0.9 × recommending more CPUs than were requested. The cap is present in the working system and must survive the port. -**`digest_user` and multi-user digests.** Merge only rows whose `User` matches `digest_user`. -Another person's peak for a same-named job would corrupt this user's recommendation with no error -anywhere. A lab-wide digest is a legitimate input; guessing whose rows to keep is not. +## Where the weekly digest comes from + +The digest is **not** something these skills generate. At Whitehead it arrives on its own: users +who ran Slurm jobs are sent a usage digest **by email, once a week**, listing each job's requested +versus used memory and CPU. `slurm-digest` is the merge step for that email — you paste its table +in, along with the week-ending date, and it folds the numbers into your sizing table. + +The practical consequence: **both skills are inert until the first digest is merged.** A fresh +install has an empty table, so every job is "unknown" — which `slurm-sizing` reports as a reason to +measure, never as licence to guess. If your site does not send such a digest, the skills have no +input and there is nothing to configure; see "Declining" below. + +## Determining the local cluster — one procedure, used everywhere + +`slurm-sizing` (its cluster gate), `slurm-digest` (its `sacct` enrichment guard) and bootstrap all +need the **local** Slurm cluster's name. They all use exactly this procedure — do not substitute +another: + +1. `sacctmgr -n -P list cluster format=Cluster` — take the single name it prints. +2. If that prints nothing or fails: `scontrol show config | grep ClusterName` and take the value + to the right of the `=`, trimmed of whitespace. +3. If neither yields a name, the local cluster is **unknown**. Say so and stop. "Unknown" is never + treated as a match. + +**Never use `hostname` for this.** It returns a node's name, not a Slurm cluster's name; the two +are different strings on essentially every site, so comparing a hostname against `digest_cluster` +fails silently and permanently disables whatever it was guarding. + +## `digest_cluster` must be the exact Slurm cluster name + +`digest_cluster` is **exact-matched** — against the local cluster name from the procedure above, +and against the `cluster` column of every log row. A colloquial or approximate answer ("the +cluster", "our HPC", a hostname, a capitalised variant) is not a failure that announces itself: it +makes `slurm-sizing`'s gate never match, so the skill goes inert forever, or it makes the digest +join never match, so every recommendation carries `>=` forever. + +Therefore **bootstrap does not ask for it in free text.** It runs the query above, shows the user +the exact string, and offers that string as the answer (see Bootstrap). And the `cluster` column +written into every log row must carry that same exact string — it is what the digest merge +filters log rows on. + +**`digest_cluster` deliberately has no default.** Guessing it is the one error that silently +corrupts results: sizing advice from the wrong cluster's data is worse than no advice, because job +IDs are not unique across clusters. Never infer it from the local hostname. + +## `digest_user` and multi-user digests + +Merge only rows whose `User` matches `digest_user`. Another person's peak for a same-named job +would corrupt this user's recommendation with no error anywhere. A lab-wide digest is a legitimate +input; guessing whose rows to keep is not. Precisely what to do when other users' rows appear — **ask once, then remember**: @@ -67,32 +121,146 @@ Precisely what to do when other users' rows appear — **ask once, then remember the Slurm account name is not the login name is exactly why this is a config key and not an inference. If no row matches `digest_user`, say so rather than merging nothing silently. -**`digest_cluster` deliberately has no default.** Guessing it is the one error that silently -corrupts results: sizing advice from the wrong cluster's data is worse than no advice, because job -IDs are not unique across clusters. If it is unset, ask the user and write their answer to the -config — never infer it from the local hostname, which is exactly the case where the answer may -differ. +## Declining — `"enabled": false` + +Not everyone has a digest, and not every machine is a cluster. A user must be able to say "this +does not apply to me" **once** and never be asked again. -**Determining the local cluster:** `sacctmgr -n -P list cluster format=Cluster`, falling back to -`scontrol show config | grep ClusterName`. If neither works, treat the local cluster as unknown and -say so rather than proceeding. +- Bootstrap always offers a decline branch ("not applicable / I have no weekly digest"). +- Declining writes `{"enabled": false}` to `~/.claude/slurm-sizing/config.json` and creates + nothing else — no table, no log, no archive directory. +- **`slurm-sizing` honours it by doing nothing**: when `enabled` is `false` it does not consult, + does not log, and does **not** prompt. It is silent, permanently, until the user changes the + file. +- **`slurm-digest` honours it too**, but it is only reachable by an explicit user invocation, so + it reports that the system is disabled and offers to re-enable (set `enabled` to `true` and run + bootstrap). It merges nothing unless the user confirms. -**Path handling.** JSON values (`table`, `log`, `archive`) may use `~` and must be expanded before -use; create parent directories as needed when bootstrapping. +Re-enabling is editing one word in the config, and either skill will do it on request. + +## Path handling + +JSON values (`table`, `log`, `archive`) may use `~` and must be expanded before use; create parent +directories as needed when bootstrapping. The config file's own location is fixed at +`~/.claude/slurm-sizing/config.json` — only the data paths are configurable. ## Bootstrap (first run, nothing exists yet) -- **Config missing:** ask the user for `digest_cluster`, write the file with that value and the - defaults above. **Before writing defaults, check for a pre-plugin layout** — `~/.claude/slurm-sizing.md`, - `~/.claude/slurm-jobs.tsv`, `~/.claude/slurm-digests/`. Those are where an early hand-rolled - version kept its data. If any exist, offer to point the config at them instead of creating empty - files beside them. Silently starting fresh next to a populated table would strand real history. -- **Table missing:** create it with the header and the "How to read a row" prose, and zero data - rows. An empty table is valid — it means every job is unknown, and an unknown job is a reason to - measure, not to size from habit. -- **Log missing:** create it with the `#` comment header explaining the `(cluster, jobid)` join key, - then the column header `cluster jobid submitted job_name scope cwd` (tab-separated). -- **Archive dir missing:** create it. +**Config missing:** + +1. **Check for a pre-plugin layout first** — `~/.claude/slurm-sizing.md`, `~/.claude/slurm-jobs.tsv`, + `~/.claude/slurm-digests/`. Those are where an early hand-rolled version kept its data. If any + exist, offer to point the config at them instead of creating empty files beside them. Silently + starting fresh next to a populated table would strand real history. +2. **Determine `digest_cluster` by running the query, not by asking in free text.** Run the + "Determining the local cluster" procedure above, show the user the exact string it returned, and + offer it as the answer — e.g. "Slurm reports this cluster's name as ``; is that the + cluster your weekly digest covers? [use it / enter a different name / not applicable]". + - If the query returns nothing, say so and ask the user for the exact Slurm cluster name (the + value `sacctmgr` would print on that cluster), warning that it is exact-matched. + - If the user's digest covers a *different* cluster than the local one, take their name — but + record it verbatim, and note that `slurm-sizing` will then be inert on this machine by design. +3. **Offer the decline branch** in the same question ("not applicable / I have no weekly digest"). + Declining writes `{"enabled": false}` and stops — see "Declining". +4. Write the file with `enabled: true`, that `digest_cluster`, and the defaults above. + +**Table missing:** create it with the header row and the "How to read a row" prose below, and zero +data rows. An empty table is valid — it means every job is unknown, and an unknown job is a reason +to measure, not to size from habit. + +**Log missing:** create it with the `#` comment header explaining the `(cluster, jobid)` join key, +then the column header `cluster jobid submitted job_name scope cwd` +(tab-separated). The `cluster` value written into each row must be the exact `digest_cluster` +string (see above). + +**Archive dir missing:** create it. Never bootstrap silently — report each file created, because a user who expected existing data needs to know the skill did not find it. + +### Table file format (`table.md`) — write exactly this on bootstrap + +The table is a GitHub-flavoured markdown table with **nine** columns, in this order. Both skills +read and write these column names; inventing a different header breaks the merge silently. + +```markdown +# Slurm sizing table + +Measured memory and CPU usage per job name, merged from weekly usage digests by +`/session-tools:slurm-digest` and read by `slurm-sizing` before a job is submitted. + +| job_name | peak_MB | peak_G | n | scope | last_seen | rec_mem | rec_cpu | notes | +|---|---|---|---|---|---|---|---|---| +``` + +**How to read a row** + +- **`job_name`** — the Slurm `JobName` exactly as it appears in the digest. This is the lookup key; + `slurm-sizing` matches the job it is about to submit against this column. +- **`peak_MB`** — the running maximum observed memory in **megabytes, at full precision**, across + every digest ever merged. **This is the authoritative memory number**; everything else about + memory is derived from it. It only ever rises (see `peak_G`). +- **`peak_G`** — `peak_MB / 1024`, rounded to 1 decimal, **for human reading only**. Never compute + a recommendation from this column: it has already lost precision, and re-deriving `peak_MB` from + it is not possible. +- **`n`** — how many weekly digests have contributed to this row. The confidence signal: `n = 1` is + one observation, not a characterisation. Re-merging a digest inflates it, which is why an + already-archived date is refused. +- **`scope`** — a short, concrete description of the workload the peak was measured at (e.g. + "12 tasks x 1565 tiles (FULL)"). Empty or `unknown` means the peak is only a **lower bound** — + nothing is known about what it was processing. A `derived: ` value is evidence + recovered from `sacct`, not a human-declared scope, and still counts as unknown until confirmed. +- **`last_seen`** — the week-ending date of the most recent digest that contained this job. +- **`rec_mem`** — the recommended `--mem`, computed **from `peak_MB`**. A `>=` prefix means the + scope was unknown: the value may justify *raising* a request, never *lowering* one. +- **`rec_cpu`** — the recommended `--cpus-per-task`, capped at the job's own `ReqCPU`. +- **`notes`** — free text, **additive**: appending a note must preserve what is already there. + `IO-BOUND` in this column is a load-bearing flag, not a comment — see below. + +**The memory columns and how they change** + +On every merge, for each job name: + +``` +peak_MB = max(existing peak_MB, this week's max UsedMem_MB) # full precision, MB, never lowered +peak_G = round(peak_MB / 1024, 1) # display only +rec_mem = round_up_to(mem_round_gb, + max(mem_floor_gb, margin_multiplier * peak_MB / 1024)) +``` + +`rec_mem` is a **pure function of the stored `peak_MB`** and the `policy` values. It is **not** +computed from this week's digest rows, and **not** from `peak_G`. + +**`rec_mem` must never decrease while `peak_MB` is unchanged.** This is the property the whole +table exists to guarantee, and it is the reason `peak_MB` is stored at all. Without it, a job whose +peak was 14963.82 MB one week (→ `peak_G 14.6`, `rec_mem 32G`) and which runs a smaller input the +next week (5000 MB) would keep `peak_G 14.6` but recompute `rec_mem` down to `12G` from this week's +smaller number — a recommendation below the row's own recorded peak, and an OOM on the next real +run. Recomputing from the stored `peak_MB` makes that arithmetically impossible. + +If a legacy table has no `peak_MB` column, add it and seed it as `peak_G * 1024` — a lossy but +never-lower starting point — say so in the report, and treat every such row's `rec_mem` as a lower +bound until the next digest refreshes it. + +**The `IO-BOUND` flag: how it is set, and what it does** + +`sacct`'s `MaxRSS` on a job that reads or writes a lot of data includes kernel page cache charged +to that job's cgroup. It is not memory the job demanded, and sizing from it can overstate the real +requirement by a large multiple. `IO-BOUND` in `notes` marks a row whose numbers are hand-pinned +from a real in-process measurement instead. + +*Setting it* — this cannot be done from digest data, by construction: + +1. Obtain **in-process** evidence of the job's real peak RSS: `resource.getrusage(...).ru_maxrss` + inside the job, a `psutil`-style RSS sampler, or a memory profiler. `sacct`/digest numbers are + exactly what the flag exists to distrust, so they can never justify setting it. +2. Set the row's `peak_MB` to that measured peak, and let `peak_G` / `rec_mem` follow from it by + the formulas above. +3. **Append** `IO-BOUND` to the `notes` column, keeping any existing note text, and say where the + measurement came from — e.g. `IO-BOUND (getrusage peak 3.2G, 2026-08-03); CPUPct 45`. + +*What it does* — `slurm-digest` leaves an `IO-BOUND` row alone: its peak is not merged, its +`rec_mem`/`rec_cpu` are not recomputed, and its `notes` are not rewritten. `slurm-sizing` trusts +the pinned row over any `sacct`-derived figure for the same job. + +*Removing it* requires the same class of evidence that set it — never a digest, never `sacct`. From 47d430018d5581b0c309c64a03f13182476f3caa Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Mon, 3 Aug 2026 16:22:10 -0400 Subject: [PATCH 03/15] fix(session-tools): executable layout probe, correct migration seed, IO-BOUND exception Three residuals from the scoped re-review. - N1: restore an executable pre-plugin layout probe. Dropping Bash(ls *) left bootstrap unable to test ~/.claude/slurm-digests/, which is a DIRECTORY that Read cannot distinguish from a missing path. Added Bash(test *) to slurm-sizing's allowed-tools and made the probe explicit: test -f on the two files, test -d on the directory. slurm-digest is unaffected (its mkdir -p is idempotent). - N4: the migration seed peak_G * 1024 was NOT "never lower". peak_G rounds to nearest, so the bare seed can sit up to 51.2 MB below the true peak: a true peak of 16394 MB displays as 16.0, giving a seeded rec_mem of 32G where 36G is correct -- a 4G drop in exactly the direction peak_MB exists to prevent. Corrected to (peak_G + 0.05) * 1024, the top of the rounding interval, and the false claim is gone from both files that carried it. - N5: "peak_MB only ever rises" / "never lower peak_MB" contradicted the IO-BOUND procedure, which lowers it on purpose when the sacct figure was inflated. Both statements now scope the guarantee to merging and name the human-set IO-BOUND correction as the single exception. Also drops a dangling self-reference in Step 2 to ReqMem(M)/UsedMem(M) forms that no longer appear elsewhere in the file after the M3 rewrite. --- .../skills/slurm-digest/SKILL.md | 18 ++++++---- .../skills/slurm-sizing/SKILL.md | 9 ++--- .../skills/slurm-sizing/reference/config.md | 33 ++++++++++++++----- 3 files changed, 41 insertions(+), 19 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index c7799b5..98ef5fe 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -63,9 +63,9 @@ for what each one means and the exact header to write if the table has to be cre 2. **Parse.** Read the pasted table. Columns are, in order: `User`, `JobID`, `JobName`, `ReqMem`, `UsedMem`, `MemPct`, `ReqCPU`, `UsedCPU`, `CPUPct`, `Elapsed`. Memory columns are in - **megabytes** — written `ReqMem(M)` / `UsedMem(M)` elsewhere in this file to state the unit, - which is **not** part of the header text; do not require the literal `(M)` to be present, and do - not reject a digest over it. Skip the `OVERALL` line, skip `bash`, `python3`, and any bare shell + **megabytes**. Where a `(M)` suffix appears on those column names, it states the unit and is + **not** part of the header text: do not require the literal `(M)` to be present, and do not + reject a digest over it. Skip the `OVERALL` line, skip `bash`, `python3`, and any bare shell or interpreter name. 3. **Filter to `digest_user` — a consent-gated filtering procedure, not part of parsing.** @@ -210,8 +210,11 @@ for what each one means and the exact header to write if the table has to be cre display `peak_G = 4794` where `4.7` belongs). Add to `n`, set `last_seen` to the digest's week-ending date. **Never lower `peak_MB`** — it is a running maximum across all weeks ever merged, so a week in which the job ran a smaller input leaves it untouched. - If a row predates the `peak_MB` column, seed it as `peak_G * 1024` (lossy but never lower), - say so in the report, and treat its `rec_mem` as a lower bound until the next digest. + If a row predates the `peak_MB` column, seed it as **`(peak_G + 0.05) * 1024`** — `peak_G` is + rounded to *nearest*, so a bare `peak_G * 1024` can sit up to 51.2 MB below the true peak and + would lower the recommendation on migration (true peak 16394 MB shows as `peak_G 16.0`: true + `rec_mem` 36 G, bare-seeded `rec_mem` 32 G). Say so in the report, and treat its `rec_mem` as a + lower bound until the next digest. **Skip rows flagged `IO-BOUND`.** That flag lives in the `notes` column of the job's **existing row in the sizing table** (the configured `table` path) — never in the incoming digest, which carries no such marker. Check the current table row before merging, not the @@ -307,7 +310,10 @@ for what each one means and the exact header to write if the table has to be cre - Advisory only. Do not edit job scripts, launcher scripts, or any pipeline code. Do not submit, cancel, or modify jobs. -- Never lower `peak_MB`; never remove an `IO-BOUND` flag without in-process evidence +- Never lower `peak_MB` **by merging** — it is a running maximum, and a smaller week leaves it + untouched. The one exception is a human setting the `IO-BOUND` flag on in-process evidence + (a deliberate correction of an inflated `sacct` figure, not a merge), which may lower it. + Never remove an `IO-BOUND` flag without in-process evidence (`getrusage`/`psutil`-style RSS sampling), because `sacct` MaxRSS cannot distinguish demand from page cache. - If the digest's column layout differs from Step 2's order, stop and report rather than guessing. diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index 5271170..68e15ce 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -12,7 +12,7 @@ description: >- about Slurm resource sizing. SKIP only when no cluster job is involved at any remove, or when the user has turned this system off (`"enabled": false` in its config). user-invocable: true -allowed-tools: Read, Write, Edit, Bash(mkdir *), Bash(sacctmgr *), Bash(scontrol *), Bash(grep *) +allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(sacctmgr *), Bash(scontrol *), Bash(grep *) --- # Slurm Job Sizing @@ -122,9 +122,10 @@ attaching to another cluster's job. Full file formats, the literal table header, and the rationale are in [reference/config.md](reference/config.md) → "Bootstrap". The branches, so none gets skipped: -- **Config missing:** check first for a pre-plugin layout (`~/.claude/slurm-sizing.md`, - `~/.claude/slurm-jobs.tsv`, `~/.claude/slurm-digests/`); if any exist, offer to point the new - config at them instead of starting empty files beside real history. Then establish +- **Config missing:** check first for a pre-plugin layout — probe with `test`, since one of the + three is a directory: `test -f ~/.claude/slurm-sizing.md`, `test -f ~/.claude/slurm-jobs.tsv`, + `test -d ~/.claude/slurm-digests`. If any exist, offer to point the new config at them instead of + starting empty files beside real history. Then establish `digest_cluster` — **do not ask for it in free text.** Run the §1 query, show the user the exact string it returned, and offer it as the answer, along with the option to name a different cluster and the option to decline ("not applicable / I have no weekly digest"). Declining writes diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index edbac8b..8502e76 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -148,10 +148,18 @@ directories as needed when bootstrapping. The config file's own location is fixe **Config missing:** -1. **Check for a pre-plugin layout first** — `~/.claude/slurm-sizing.md`, `~/.claude/slurm-jobs.tsv`, - `~/.claude/slurm-digests/`. Those are where an early hand-rolled version kept its data. If any - exist, offer to point the config at them instead of creating empty files beside them. Silently - starting fresh next to a populated table would strand real history. +1. **Check for a pre-plugin layout first** — two files and one **directory**, where an early + hand-rolled version kept its data. Probe all three with `test`, which answers for a directory as + cleanly as for a file (a `Read` cannot: it fails on a missing path and on a directory alike): + + ```bash + test -f ~/.claude/slurm-sizing.md && echo "found: legacy table" + test -f ~/.claude/slurm-jobs.tsv && echo "found: legacy log" + test -d ~/.claude/slurm-digests && echo "found: legacy archive dir" + ``` + + If any exist, offer to point the config at them instead of creating empty files beside them. + Silently starting fresh next to a populated table would strand real history. 2. **Determine `digest_cluster` by running the query, not by asking in free text.** Run the "Determining the local cluster" procedure above, show the user the exact string it returned, and offer it as the answer — e.g. "Slurm reports this cluster's name as ``; is that the @@ -199,7 +207,10 @@ Measured memory and CPU usage per job name, merged from weekly usage digests by `slurm-sizing` matches the job it is about to submit against this column. - **`peak_MB`** — the running maximum observed memory in **megabytes, at full precision**, across every digest ever merged. **This is the authoritative memory number**; everything else about - memory is derived from it. It only ever rises (see `peak_G`). + memory is derived from it. **It never falls through merging** — a week with a smaller input + leaves it untouched. The single exception is a human setting the `IO-BOUND` flag on in-process + evidence (below), which may lower it: that is a deliberate correction of an inflated `sacct` + figure, not a merge. - **`peak_G`** — `peak_MB / 1024`, rounded to 1 decimal, **for human reading only**. Never compute a recommendation from this column: it has already lost precision, and re-deriving `peak_MB` from it is not possible. @@ -222,7 +233,7 @@ Measured memory and CPU usage per job name, merged from weekly usage digests by On every merge, for each job name: ``` -peak_MB = max(existing peak_MB, this week's max UsedMem_MB) # full precision, MB, never lowered +peak_MB = max(existing peak_MB, this week's max UsedMem_MB) # full precision, MB; a merge never lowers it peak_G = round(peak_MB / 1024, 1) # display only rec_mem = round_up_to(mem_round_gb, max(mem_floor_gb, margin_multiplier * peak_MB / 1024)) @@ -238,9 +249,13 @@ next week (5000 MB) would keep `peak_G 14.6` but recompute `rec_mem` down to `12 smaller number — a recommendation below the row's own recorded peak, and an OOM on the next real run. Recomputing from the stored `peak_MB` makes that arithmetically impossible. -If a legacy table has no `peak_MB` column, add it and seed it as `peak_G * 1024` — a lossy but -never-lower starting point — say so in the report, and treat every such row's `rec_mem` as a lower -bound until the next digest refreshes it. +If a legacy table has no `peak_MB` column, add it and seed it as **`(peak_G + 0.05) * 1024`**. The +`+ 0.05` is not padding, it is the rounding correction: `peak_G` is rounded to **nearest**, so a +bare `peak_G * 1024` can sit up to 51.2 MB *below* the true peak and migration would then lower a +recommendation — e.g. a true peak of 16394 MB displays as `peak_G 16.0`, whose true `rec_mem` is +36 G but whose bare-seeded `rec_mem` is 32 G, a 4 G drop in exactly the direction this column +exists to prevent. Seeding at the top of the rounding interval cannot go low. Say so in the report, +and treat every such row's `rec_mem` as a lower bound until the next digest refreshes it. **The `IO-BOUND` flag: how it is set, and what it does** From 1362bf4f91e34d512732eef222506427162a7c58 Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Tue, 4 Aug 2026 10:26:10 -0400 Subject: [PATCH 04/15] fix(session-tools): define rec_cpu row selection, solicit digest_user, complete allowed-tools Three defects from a sandbox end-to-end execution of slurm-digest by a fresh agent, plus two minors from the same run. - F1: rec_cpu was undefined when one job name has several rows in a digest. A real name appearing 9 times spanned CPUPct 1.55..49.08; pairing the CPU figures with the peak-MEMORY row gave rec_cpu 2 where the peak-CPU row gives 12 -- a 6x spread with nothing in the text to choose between them, and the low answer would serialise a job that has used ~8 cores. Ruled: take the row with the MAXIMUM CPUPct and pair it with that same row's ReqCPU. The peak-memory and peak-CPU runs are generally different runs; under-provisioning CPU is the harmful direction; and this matches the running-max the memory side already uses. Stated in slurm-digest Step 9 and in config.md. - F2: digest_user was written into the config from $USER without ever being shown. config.md argues this must be a config key precisely because the Slurm account and login name can differ -- and for such a user the wrong value filters out every one of their own rows, merging nothing with no error. Bootstrap now shows the value, names $USER as its source, states it must match the digest's User column, and offers correction. - F3: sixth instance of the allowed-tools class. slurm-digest runs the same bootstrap as slurm-sizing but did not get the Bash(test *) added there for the pre-plugin layout probe. Added, the delegation to that probe is now explicit in the body, and both skills were re-audited command-by-command. Minors: the parse step's "any bare shell or interpreter name" is now an enumerated, explicitly extensible list; and a blank CPUPct is defined as absence of evidence (excluded from the maximum), not as zero, which would drive rec_cpu to the floor on no data. --- .../skills/slurm-digest/SKILL.md | 40 ++++++++++++++++--- .../skills/slurm-sizing/SKILL.md | 4 ++ .../skills/slurm-sizing/reference/config.md | 25 ++++++++++-- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index 98ef5fe..78c96f1 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -6,7 +6,7 @@ description: >- week-ending date it covers, then run it explicitly; this skill does not trigger on its own the way slurm-sizing does. user-invocable: true -allowed-tools: Read, Write, Edit, Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(sacct *), Bash(sacctmgr *), Bash(scontrol *) +allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(sacct *), Bash(sacctmgr *), Bash(scontrol *) --- # Slurm Digest Merge @@ -30,7 +30,11 @@ local cluster" procedure, and the multi-user filtering rules referenced in Step [../slurm-sizing/reference/config.md](../slurm-sizing/reference/config.md). Read `enabled`, `digest_cluster`, `digest_user`, `table`, `log`, `archive`, `multi_user_digest`, and `policy` from `~/.claude/slurm-sizing/config.json` before anything else below; if that config doesn't exist yet, -follow its "Bootstrap" section first rather than guessing any of these values. +follow its "Bootstrap" section first rather than guessing any of these values. That path runs here, +in this skill, on first use — it probes for a pre-plugin layout with `test -f ~/.claude/slurm-sizing.md`, +`test -f ~/.claude/slurm-jobs.tsv`, `test -d ~/.claude/slurm-digests` (the third is a directory, +which `Read` cannot distinguish from a missing path), confirms `digest_cluster` and `digest_user` +with the user, and creates the archive directory with `mkdir -p`. **If `enabled` is `false`**, the user has declined this system. Merge nothing. Report that it is turned off and offer to re-enable it (set `"enabled": true` and run bootstrap); proceed only if @@ -65,8 +69,13 @@ for what each one means and the exact header to write if the table has to be cre `UsedMem`, `MemPct`, `ReqCPU`, `UsedCPU`, `CPUPct`, `Elapsed`. Memory columns are in **megabytes**. Where a `(M)` suffix appears on those column names, it states the unit and is **not** part of the header text: do not require the literal `(M)` to be present, and do not - reject a digest over it. Skip the `OVERALL` line, skip `bash`, `python3`, and any bare shell - or interpreter name. + reject a digest over it. Skip the `OVERALL` summary line. Also skip rows whose `JobName` is a + bare shell or interpreter rather than a real job — these carry no workload identity, so their + peaks cannot be attributed to anything. The list, which is **extensible**: `bash`, `sh`, `zsh`, + `csh`, `tcsh`, `ksh`, `dash`, `python`, `python2`, `python3`, `perl`, `ruby`, `Rscript`, `R`, + `julia`, `node`, `interactive`, `srun`, `salloc`, `wrap`. Treat any other bare + interpreter/shell binary name the same way, and say in the report which names were skipped so a + real job that happens to be named like one is visible rather than silently dropped. 3. **Filter to `digest_user` — a consent-gated filtering procedure, not part of parsing.** Merge only rows whose `User` matches the configured `digest_user` (default: the invoking @@ -254,7 +263,19 @@ for what each one means and the exact header to write if the table has to be cre - scope unknown (empty, or a `derived:` value not yet confirmed by a human) -> prefix `>=`; the row may justify raising a request, never lowering one -9. **CPU.** One formula across the whole range — no bands, no carve-outs: +9. **CPU.** **First pick the row, then apply the formula.** A job name usually has **several rows + in one digest** (nine runs of the same name is ordinary), and their `CPUPct` values differ + widely. **Use the row with the MAXIMUM `CPUPct` for that job name, and pair it with that same + row's `ReqCPU`.** Never take the CPU figures from the peak-*memory* row: the peak-memory run and + the peak-CPU run are generally different runs, and pairing them silently mixes two measurements. + Worked case: a name appearing 9 times with `CPUPct` from 1.55 to 49.08 gives `rec_cpu = 12` from + the 49.08 row (≈7.85 of 16 cores), but `rec_cpu = 2` if the 1.55 row is used because it happened + to hold the memory peak — a 6x spread, and the low answer would serialise a job that has + demonstrably used ~8 cores. **Under-provisioning CPU is the harmful direction** (slow or serial, + not merely wasteful), so take the max — the same running-max logic the memory side already uses, + rather than a representative sample. + + Then, one formula across the whole range — no bands, no carve-outs: `rec_cpu = min(ReqCPU, max(policy.cpu_floor, ceil(ReqCPU x CPUPct / 100 x policy.cpu_headroom)))` — at the default values (`cpu_floor = 2`, `cpu_headroom = 1.5`), `rec_cpu = min(ReqCPU, max(2, ceil(ReqCPU x CPUPct/100 x 1.5)))`. **The `ReqCPU` cap is not @@ -264,7 +285,14 @@ for what each one means and the exact header to write if the table has to be cre deliberate headroom over the observed average, not a serial/parallel classification — e.g. a job at 25% of 16 requested CPUs used ~4 cores on average, so `ceil(16 x 0.25 x 1.5) = 6`. - Record the observed `CPUPct` in `notes` **additively**: `notes` is one shared free-text column + **Blank `CPUPct`.** Real digests contain rows with an empty `CPUPct` field. A blank is **not + zero** — it is absence of evidence, and treating it as `0` would drive `rec_cpu` to `cpu_floor` + on no data at all. Such a row **contributes no CPU evidence**: exclude it when taking the + maximum above. If *every* row for a job name has a blank `CPUPct`, leave `rec_cpu` unchanged + (or empty on a new row), and say so in the report rather than emitting a floor value that looks + measured. + + Record the observed maximum `CPUPct` in `notes` **additively**: `notes` is one shared free-text column that also holds the load-bearing `IO-BOUND` flag and any human annotation. Update or append the `CPUPct` fragment while **preserving every other fragment already in the cell** — never overwrite the column with a bare `CPUPct `. Use a separated form such as diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index 68e15ce..79885c0 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -130,6 +130,10 @@ Full file formats, the literal table header, and the rationale are in string it returned, and offer it as the answer, along with the option to name a different cluster and the option to decline ("not applicable / I have no weekly digest"). Declining writes `{"enabled": false}`, creates nothing else, and this skill then stays silent permanently (§0). + Then **confirm `digest_user` rather than just writing it**: show the value you intend to use, say + it came from `$USER`, state that it must match the digest's `User` column exactly, and let the + user correct it — the two can legitimately differ, and a wrong value silently filters out every + one of their own rows. - **Table missing:** create it with the nine-column header row and the "How to read a row" prose given verbatim in [reference/config.md](reference/config.md) → "Table file format", and zero data rows. An empty table is a valid, complete state — every job is unknown, which is a reason to diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index 8502e76..c4ea46b 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -38,7 +38,7 @@ verbatim, and in particular do not copy a cluster name from anyone else's config | `policy.margin_multiplier` | `2` | `rec_mem` = this × peak. | | `policy.mem_floor_gb` | `8` | Never recommend less than this. | | `policy.mem_round_gb` | `4` | Round `rec_mem` UP to a multiple of this. | -| `policy.cpu_headroom` | `1.5` | `rec_cpu` = ceil(ReqCPU × CPUPct/100 × this), capped at `ReqCPU`. | +| `policy.cpu_headroom` | `1.5` | `rec_cpu` = ceil(ReqCPU × CPUPct/100 × this), capped at `ReqCPU`. When a job name has several rows in one digest, `CPUPct` and `ReqCPU` are taken from that name's **maximum-`CPUPct`** row (below). | | `policy.cpu_floor` | `2` | Never recommend fewer CPUs than this. | **The `policy` values are tunable defaults, not invariants — but they are the reviewed defaults, @@ -47,6 +47,14 @@ never to nearest — a safety margin that rounds down is not a margin. A site th `margin_multiplier` is choosing more OOM risk in exchange for queue priority, and should say so to itself explicitly, in config, rather than by editing the skills. +**Which row supplies `CPUPct` when a job name appears several times in one digest: the row with +the MAXIMUM `CPUPct`, paired with that same row's `ReqCPU`.** Not the peak-memory row — the +peak-memory run and the peak-CPU run are generally different runs, and pairing across them mixes +two measurements. Under-provisioning CPU is the harmful direction (a serialised job, not merely a +wasteful reservation), so this takes a running max exactly as the memory side does. A row with a +**blank** `CPUPct` contributes no CPU evidence and is excluded from that maximum; blank is absence +of evidence, not `0`. + **The `ReqCPU` cap is not optional.** Full rule: `rec_cpu = min(ReqCPU, max(cpu_floor, ceil(ReqCPU × CPUPct/100 × cpu_headroom)))`. Without the cap, a job at 4 CPUs and 90% utilisation computes `ceil(4 × 0.9 × 1.5) = 6` — @@ -168,9 +176,18 @@ directories as needed when bootstrapping. The config file's own location is fixe value `sacctmgr` would print on that cluster), warning that it is exact-matched. - If the user's digest covers a *different* cluster than the local one, take their name — but record it verbatim, and note that `slurm-sizing` will then be inert on this machine by design. -3. **Offer the decline branch** in the same question ("not applicable / I have no weekly digest"). - Declining writes `{"enabled": false}` and stops — see "Declining". -4. Write the file with `enabled: true`, that `digest_cluster`, and the defaults above. +3. **Confirm `digest_user` — show it, source it, let them correct it. Do not just write it.** + The default is the invoking `$USER`, but this key exists *because* a Slurm account name and a + login name can legitimately differ; a user for whom they differ gets a silently wrong value that + then filters out **all of their own rows** — the digest merges nothing and nothing says why. + So state the intended value, where it came from, and what it has to match — e.g. "I'll filter + the digest to `User = <$USER>` (taken from your `$USER` login name). This must match the `User` + column in the digest exactly. [use it / enter a different account name]". If the user is + already holding a digest, invite them to check the `User` column against it. +4. **Offer the decline branch** in the same question as step 2 ("not applicable / I have no weekly + digest"). Declining writes `{"enabled": false}` and stops — see "Declining". +5. Write the file with `enabled: true`, that `digest_cluster`, that `digest_user`, and the defaults + above. **Table missing:** create it with the header row and the "How to read a row" prose below, and zero data rows. An empty table is valid — it means every job is unknown, and an unknown job is a reason From 2740b4cb05ace9413a7c615b91530c4d545dc097 Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Tue, 4 Aug 2026 11:28:19 -0400 Subject: [PATCH 05/15] fix(session-tools): define waste ranking and reservation delta, normalise digest array IDs Seven findings from a second sandbox execution test (F7 needs no code change). Important: - F1: Step 10 ranked by "(ReqMem - rec_mem) x n", but ReqMem is megabytes and rec_mem is a STRING in gigabytes with a G suffix and often a >= prefix -- 204800.00 - ">= 28G" is not arithmetic. Now states the parse and both unit conversions explicitly and ranks by waste_GB. This ranking decides which jobs the user is asked to scope, which is the only path from >= to a real recommendation. - F4: Step 6 assumed digest JobIDs are bare base IDs. Real digests carry array-task IDs, so the _ normalisation written for the sacct side now applies to the digest side too, stated as "suffixes appear on BOTH sides". The array bullet's contrary claim is corrected. Previously harmless only because sacct returned nothing for every fixture ID. - F5: Step 12's "reservation delta" had no formula, units or scope. Defined as sum(ReqMem_GB - rec_mem_GB) over rows merged this week, reported as TWO subtotals -- actionable (no >= prefix) and pending-scope (>= rows). One merged figure would contradict the same step's statement that >= rows are not actionable. Minor: - F2: a first-time CPU note is exactly "CPUPct "; on a populated cell it is appended after "; ", and appending never rewrites what is there. - F3: an unexpanded shell variable in a job name (${...} or a bare $) is NOT skipped -- it is a real job with real measurements -- but is reported as a probable quoting bug, since the job name is the merge key and an unexpanded name never aggregates with its intended siblings. - F6: UsedCPU holds CPU time, not a core count, and is deliberately unused. F7 (three of Step 8's four scope branches untested) is a test-coverage gap being addressed by a seeded execution test; Step 8 is unchanged, as ruled. Also replaces three real cluster job IDs that entered the F4 text from the test fixture with the invented IDs already used in that step. --- .../skills/slurm-digest/SKILL.md | 98 +++++++++++++++---- 1 file changed, 79 insertions(+), 19 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index 78c96f1..fafed77 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -69,7 +69,14 @@ for what each one means and the exact header to write if the table has to be cre `UsedMem`, `MemPct`, `ReqCPU`, `UsedCPU`, `CPUPct`, `Elapsed`. Memory columns are in **megabytes**. Where a `(M)` suffix appears on those column names, it states the unit and is **not** part of the header text: do not require the literal `(M)` to be present, and do not - reject a digest over it. Skip the `OVERALL` summary line. Also skip rows whose `JobName` is a + reject a digest over it. + + **`UsedCPU` is CPU *time*, not a core count**, despite the name — it holds durations such as + `01:56:53` or `00:08.791`. **This procedure deliberately never consumes it**; every CPU + recommendation comes from `ReqCPU` and `CPUPct` (Step 9). Do not treat it as a number of cores + in any future edit — doing so would silently produce recommendations in the wrong unit. + + Skip the `OVERALL` summary line. Also skip rows whose `JobName` is a bare shell or interpreter rather than a real job — these carry no workload identity, so their peaks cannot be attributed to anything. The list, which is **extensible**: `bash`, `sh`, `zsh`, `csh`, `tcsh`, `ksh`, `dash`, `python`, `python2`, `python3`, `perl`, `ruby`, `Rscript`, `R`, @@ -143,10 +150,19 @@ for what each one means and the exact header to write if the table has to be cre 6. **Enrich unmatched rows from `sacct` — a conditional recovery procedure, not part of the lookup.** For every digest row that did NOT match a submission-log entry in Step 5, attempt to - recover a *derived* scope directly from Slurm before falling back to `scope: unknown`. The six - bullets below are sequential and order-dependent — cluster guard, then batch query, then - discard step rows, then collapse array rows, then the miss guard, then record — treat them as - an ordered procedure to follow in full, not as loose elaboration on the lookup. + recover a *derived* scope directly from Slurm before falling back to `scope: unknown`. The seven + bullets below are sequential and order-dependent — normalise the digest's own JobIDs, cluster + guard, batch query, discard step rows, collapse array rows, the miss guard, then record — treat + them as an ordered procedure to follow in full, not as loose elaboration on the lookup. + - **Normalise the digest's own `JobID` to a base ID first — array-task suffixes appear on BOTH + sides.** Do not assume the digest carries bare base IDs; real digests routinely carry + array-task IDs such as `7932156_0`, `7932157_0`, `7932158_1`. Before using a digest `JobID` + as an enrichment lookup key, strip any `_` suffix exactly as the array bullet below + strips it from `sacct`'s rows — the same normalisation, applied to both sides, so the two + agree on what "the job" is. Querying `sacct` with an unnormalised `7932156_0` narrows the + result to one task at best and misses at worst, and the miss is silent (it looks like a + retention gap). Keep the original suffixed ID on the digest row for reporting; normalise only + the lookup key. - **Cluster guard, checked first.** `sacct` only sees the LOCAL cluster's accounting database — **verified in practice**: a job ID from the digest's cluster does not resolve via `sacct` run against a different cluster's accounting database (`sacct -L -j ` @@ -181,8 +197,10 @@ for what each one means and the exact header to write if the table has to be cre contains a `.` before doing anything else with the result; only bare (``) or array-task (`_`) rows carry real data. - **Array jobs: map task rows back to the digest's base job, then collapse to one scope.** A - digest row for an array job has a bare base `JobID` (e.g. `7932156`); `sacct` returns one row - per task (`7932156_0` … `7932156_11`) plus that task's step rows. After discarding step rows, + digest row for an array job carries either a bare base `JobID` (e.g. `7932156`) or an + array-task ID (e.g. `7932156_0`) — normalised to the base by the first bullet either way; + `sacct` returns one row per task (`7932156_0` … `7932156_11`) plus that task's step rows, + so the suffix can be present on both sides. After discarding step rows, strip the `_` suffix to correlate each surviving row back to the base job. The surviving task rows for one base job carry an identical `SubmitLine` (it's the same `sbatch` invocation) — take one as the scope. If they somehow differ, record the first and add a note @@ -292,19 +310,38 @@ for what each one means and the exact header to write if the table has to be cre (or empty on a new row), and say so in the report rather than emitting a floor value that looks measured. - Record the observed maximum `CPUPct` in `notes` **additively**: `notes` is one shared free-text column - that also holds the load-bearing `IO-BOUND` flag and any human annotation. Update or append the - `CPUPct` fragment while **preserving every other fragment already in the cell** — never - overwrite the column with a bare `CPUPct `. Use a separated form such as - `IO-BOUND (getrusage peak 3.2G); CPUPct 45` so fragments stay individually editable. + Record the observed maximum `CPUPct` in `notes` **additively**: `notes` is one shared free-text + column that also holds the load-bearing `IO-BOUND` flag and any human annotation. The two cases: + - **`notes` is empty** (a new row, or a row never annotated): write exactly `CPUPct `. + - **`notes` already has content**: append `; CPUPct ` after what is there, or update an + existing `CPUPct` fragment in place — e.g. `IO-BOUND (getrusage peak 3.2G); CPUPct 45`. The + `; ` separator keeps fragments individually editable. + + **Appending never rewrites what is already there.** Preserve every other fragment in the cell; + never replace the column with a bare `CPUPct ` when it held anything else. **`IO-BOUND` rows are exempt from this step**, as from Steps 7 and 8: do not recompute their `rec_cpu` and do not touch their `notes`. An overwritten note erases the flag, and the next week's page-cache MaxRSS then enters the peak — the exact failure the flag exists to prevent. -10. **Prompt for scope.** List unmatched job names ranked by wasted reservation - `(ReqMem - rec_mem) x n`, largest first, capped at 10. Ask the user to supply scope for those. - Rank by waste, not recency: scope on a 9-run 200 G job is worth ~1,584 G; scope on a one-off - 8 G job is worth nothing. Names left unanswered stay `unknown` and carry forward. +10. **Prompt for scope.** List unmatched job names ranked by wasted reservation, largest first, + capped at 10. Ask the user to supply scope for those. + + **Compute the ranking key explicitly — the two inputs are in different units and one of them + is a string.** `ReqMem` is in **megabytes** (Step 2); `rec_mem` is a **string in gigabytes** + carrying a `G` suffix and often a `>=` prefix, so `204800.00 - ">= 28G"` is not arithmetic. + Normalise both to GB first: + + ``` + rec_mem_GB = number(rec_mem with any ">=" prefix and the trailing "G" stripped) # e.g. ">= 28G" -> 28 + ReqMem_GB = ReqMem / 1024 # MB -> GB + waste_GB = (ReqMem_GB - rec_mem_GB) x n + ``` + + Rank by `waste_GB` — **units are gigabytes** — not by recency: scope on a 9-run 200 G job is + worth ~1,584 G; scope on a one-off 8 G job is worth nothing. This ranking decides which jobs + the user is asked about, which is the only mechanism that turns `>=` rows into real + recommendations, so a mis-ordered list has a real cost. Names left unanswered stay `unknown` + and carry forward. **Show derived evidence alongside the prompt.** For any unmatched name Step 6 enriched with a `derived:` scope, display that derived submit line next to it so the human can confirm or correct it. **Both confirmation and correction promote it identically:** whether the human @@ -323,9 +360,32 @@ for what each one means and the exact header to write if the table has to be cre operation. 12. **Report.** State: the week-ending date merged, rows parsed, rows dropped as capped, scope - matches from the log, job names updated, job names new, peaks that rose, and the total - reservation delta if the recommendations were applied. Be explicit that `>=` rows are not - actionable for sizing down. + matches from the log, job names updated, job names new, and peaks that rose. + + **Reservation delta — define it, and report it as TWO figures.** Over the rows merged this + week, using the same GB normalisation as Step 10 (`ReqMem_GB = ReqMem / 1024`; `rec_mem_GB` = + `rec_mem` with any `>=` prefix and trailing `G` stripped): + + ``` + delta_GB = sum over merged rows of (ReqMem_GB - rec_mem_GB) + ``` + + Report it split, never as one number: + - **Actionable subtotal** — summed over rows whose `rec_mem` has **no** `>=` prefix. This is + reservation that can actually be given back now. + - **Pending-scope subtotal** — summed over rows whose `rec_mem` **does** carry `>=`. This is + not actionable; it is what capturing scope would unlock. + + Merging the two into a single figure would contradict this step's own statement that `>=` rows + are not actionable for sizing down, and the split is the more useful number anyway. Say the + units (GB) and be explicit that `>=` rows are not actionable for sizing down. + + **Flag job names that look like an unexpanded shell variable.** If any merged job name contains + `${` or a bare `$` (e.g. a name like `${USER}_probe`), call it out as a probable quoting bug in + the submitting script — the name reached Slurm unexpanded. Do **not** skip or drop such a row: + it is a real job and its measurements are real, so it merges normally. But the job name is this + system's merge key, so an unexpanded name will never aggregate with the sibling runs it was + meant to share a row with, and the user should know that before trusting its `n`. Also state: how many unmatched rows were enriched with a `derived:` scope from `sacct`, how many attempted enrichments missed (empty `SubmitLine` / retention gap), and whether enrichment was skipped entirely because the digest's cluster differs from the local cluster, From de9162ce16e24f371c32d4e1356c6fb6f8c88ee8 Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Tue, 4 Aug 2026 11:39:13 -0400 Subject: [PATCH 06/15] fix(session-tools): replace the 2x memory margin with headroom_frac (default 0.30) A second digest at ~45% overall memory utilisation -- versus the ~8% one this system was calibrated on -- exposed a policy defect. The flat 2x margin overshoots once utilisation is high: 6 of 27 job names got a rec_mem HIGHER than what they had requested, e.g. a 512G request that peaked at 341.8G (well sized) was told to ask for 684G. Because an unknown-scope row "may justify raising a request", those are actionable, so the tool would push already-well-sized jobs upward -- spending queue time and per-user memory cap, which is the exact harm it exists to prevent, inverted. rec_mem = roundup_to(mem_round_gb, max(mem_floor_gb, peak_GB * (1 + headroom_frac))) - policy.margin_multiplier is REMOVED; policy.headroom_frac (default 0.30) replaces it. mem_floor_gb (8) and mem_round_gb (4, still rounding UP) keep their meanings. - Measured: raises-above-request 6/27 -> 1/27 on the high-utilisation digest, unchanged (2/65) on the low-utilisation one. 341.8G peak: 684G -> 448G. Three things a future editor would get wrong, now stated in both files: 1. peak_GB * 1.30 IS a 1.3x multiplier. The two forms differ in size, not in kind. Said plainly so nobody "restores" 2x believing they differ. 2. The margin applies to a running max -- 30% above the worst reading ever seen for that job name, not above a typical week. That is what makes it defensible. 3. Recommending above the request is still possible and still intended; what was removed is the systematic overshoot, not the ability to flag real under-provisioning. Every worked example that hardcoded a 2x result is recomputed: the Step 8 example (13.2G -> 20G, not 28G), the monotonicity example (14963.82MB -> 20G, and 8G if wrongly recomputed from a 5000MB week), and the migration-seed example, which had to be replaced outright -- under 1.3x the old 16394MB case no longer crosses a 4G rounding boundary, so it demonstrated nothing. The new 9462MB case does (16G true vs 12G bare-seeded). rec_cpu, the scope rules, the duplicate gate, peak_MB monotonicity and the enrichment procedure are untouched. --- .../skills/slurm-digest/SKILL.md | 42 ++++++++++----- .../skills/slurm-sizing/reference/config.md | 52 ++++++++++++++----- 2 files changed, 70 insertions(+), 24 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index fafed77..14bd0c0 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -238,10 +238,10 @@ for what each one means and the exact header to write if the table has to be cre week-ending date. **Never lower `peak_MB`** — it is a running maximum across all weeks ever merged, so a week in which the job ran a smaller input leaves it untouched. If a row predates the `peak_MB` column, seed it as **`(peak_G + 0.05) * 1024`** — `peak_G` is - rounded to *nearest*, so a bare `peak_G * 1024` can sit up to 51.2 MB below the true peak and - would lower the recommendation on migration (true peak 16394 MB shows as `peak_G 16.0`: true - `rec_mem` 36 G, bare-seeded `rec_mem` 32 G). Say so in the report, and treat its `rec_mem` as a - lower bound until the next digest. + rounded to *nearest*, so a bare `peak_G * 1024` can sit up to 51.2 MB below the true peak and, + when that crosses a `mem_round_gb` boundary, lowers the recommendation on migration by a full + 4 G (true peak 9462 MB shows as `peak_G 9.2`: true `rec_mem` 16 G, bare-seeded `rec_mem` 12 G). + Say so in the report, and treat its `rec_mem` as a lower bound until the next digest. **Skip rows flagged `IO-BOUND`.** That flag lives in the `notes` column of the job's **existing row in the sizing table** (the configured `table` path) — never in the incoming digest, which carries no such marker. Check the current table row before merging, not the @@ -250,23 +250,41 @@ for what each one means and the exact header to write if the table has to be cre untouched** by Steps 7, 8 and 9 — peak, recommendations and notes alike. 8. **Recompute `rec_mem` from the stored `peak_MB`.** - `rec_mem = policy.margin_multiplier x (peak_MB / 1024)` (default `margin_multiplier = 2`), + `rec_mem = (peak_MB / 1024) x (1 + policy.headroom_frac)` (default `headroom_frac = 0.30`), where `peak_MB` is **the row's stored full-precision peak after Step 7** — **not** this week's digest rows, and **not** the 1-decimal `peak_G` displayed in the table. Floor the result at `policy.mem_floor_gb` (default 8 G), then round UP to the next multiple of `policy.mem_round_gb` (default 4 G) — never down; this is a safety margin, so a peak that lands exactly on a multiple of `mem_round_gb` stays there, and any remainder pushes to the next multiple up. Worked example - at the default values: a stored peak of 13.2 G gives `2 x 13.2 = 26.4`, which rounds up to - 28 G, not down to 24 G. + at the default values: a stored peak of 13.2 G gives `13.2 x 1.30 = 17.16`, which rounds up to + 20 G, not down to 16 G. + + **The margin applies to a running max, not a single observation.** `peak_MB` is the maximum + across every digest ever merged for this job name, so this is 30% above the **worst reading ever + seen** for it, not 30% above a typical week. That is what makes a margin this small defensible. **`rec_mem` must never decrease while `peak_MB` is unchanged.** Because it is a pure function of the stored `peak_MB` and the `policy` values, this holds automatically — but it is the property to check if the arithmetic is ever changed. Computing it from *this week's* peak instead would - break it: a job that peaked at 14963.82 MB (`peak_G 14.6`, `rec_mem 32G`) and then ran a smaller - input peaking at 5000 MB would keep `peak_G 14.6` but drop to `rec_mem 12G` — a recommendation - below the row's own recorded peak, and an OOM on the next full-size run. - - **These are the reviewed defaults, not hardcoded law** — a site that lowers `margin_multiplier`, + break it: a job that peaked at 14963.82 MB (`peak_G 14.6`, `rec_mem 20G`) and then ran a smaller + input peaking at 5000 MB would keep `peak_G 14.6` but drop to `rec_mem 8G` — the floor, less + than half the row's own recorded peak, and an OOM on the next full-size run. + + **`headroom_frac` is a smaller margin than the `2x` it replaced — not a different kind of + rule.** `peak_GB x 1.30` **is** a 1.3x multiplier; do not read the fractional form as + categorically safer. The old `2x` was calibrated on jobs running at 3–15% memory utilisation; + against a digest of jobs near 45% utilisation it recommended **more than the job had requested + for 6 of 27 job names** (e.g. a 512 G request that peaked at 341.8 G — well sized — was told to + ask for 684 G). Because an unknown-scope row may justify *raising* a request, those were + actionable, so the tool pushed well-sized jobs upward and spent queue time and per-user memory + cap to do it. At `0.30` that job gets 448 G and the raises-above-request rate falls to 1 of 27, + unchanged on low-utilisation digests. **Do not "restore" `2x`.** + + **A recommendation above the request is still possible, and still intended.** What was removed + is the *systematic* overshoot on well-utilised jobs, not the ability to flag a job that + genuinely needs more than it asked for. + + **These are the reviewed defaults, not hardcoded law** — a site that lowers `headroom_frac`, or any other `policy` value, in config is choosing more OOM risk in exchange for queue priority, and should make that choice deliberately, in config, not by editing this file. diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index c4ea46b..e9228c2 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -13,7 +13,7 @@ read it. If it does not exist, the skills bootstrap it (below). "archive": "~/.claude/slurm-sizing/digests", "multi_user_digest": false, "policy": { - "margin_multiplier": 2, + "headroom_frac": 0.30, "mem_floor_gb": 8, "mem_round_gb": 4, "cpu_headroom": 1.5, @@ -35,7 +35,7 @@ verbatim, and in particular do not copy a cluster name from anyone else's config | `log` | `~/.claude/slurm-sizing/jobs.tsv` | The submission log where scope is recorded. | | `archive` | `~/.claude/slurm-sizing/digests` | Directory of raw archived digests, one per week-ending date. | | `multi_user_digest` | `false` | This digest is known to contain other users' rows and should be filtered without halting. | -| `policy.margin_multiplier` | `2` | `rec_mem` = this × peak. | +| `policy.headroom_frac` | `0.30` | `rec_mem` = `peak_GB × (1 + this)`, floored and rounded up (below). | | `policy.mem_floor_gb` | `8` | Never recommend less than this. | | `policy.mem_round_gb` | `4` | Round `rec_mem` UP to a multiple of this. | | `policy.cpu_headroom` | `1.5` | `rec_cpu` = ceil(ReqCPU × CPUPct/100 × this), capped at `ReqCPU`. When a job name has several rows in one digest, `CPUPct` and `ReqCPU` are taken from that name's **maximum-`CPUPct`** row (below). | @@ -44,9 +44,28 @@ verbatim, and in particular do not copy a cluster name from anyone else's config **The `policy` values are tunable defaults, not invariants — but they are the reviewed defaults, and changing them changes the safety properties.** In particular `mem_round_gb` rounds **UP**, never to nearest — a safety margin that rounds down is not a margin. A site that lowers -`margin_multiplier` is choosing more OOM risk in exchange for queue priority, and should say so to +`headroom_frac` is choosing more OOM risk in exchange for queue priority, and should say so to itself explicitly, in config, rather than by editing the skills. +**Why `headroom_frac` and not a multiplier — read this before changing it back.** `peak_GB × 1.30` +**is** arithmetically a 1.3x multiplier; the two forms are not different in kind, only in size. +The margin used to be `2x`, and that was calibrated against jobs running at roughly 3–15% memory +utilisation, where doubling a small peak is cheap. Measured against a digest whose jobs ran near +45% utilisation, `2x` recommended **more memory than the job had requested for 6 of 27 job names** — +e.g. a job that requested 512 G, peaked at 341.8 G and was therefore well sized got a 684 G +recommendation. Since an unknown-scope row "may justify raising a request", those are actionable, +so the tool would have pushed already-well-sized jobs *upward*, costing queue time and per-user +memory cap: the exact harm it exists to prevent, inverted. At `headroom_frac = 0.30` that same job +gets 448 G, and raises-above-request fall to 1 of 27 on the high-utilisation digest while staying +unchanged on the low-utilisation one. **Do not "restore" `2x` on the belief that a fractional +headroom is a categorically safer construction — it is the same construction with a smaller +number, and the smaller number is the point.** + +**A recommendation above the request is still possible, and still intended.** The change removes a +*systematic* overshoot on well-utilised jobs; it does not remove the tool's ability to say that a +job genuinely needs more than it asked for. If a peak plus 30% exceeds the request, that is a real +signal about under-provisioning, not a bug to clamp away. + **Which row supplies `CPUPct` when a job name appears several times in one digest: the row with the MAXIMUM `CPUPct`, paired with that same row's `ReqCPU`.** Not the peak-memory row — the peak-memory run and the peak-CPU run are generally different runs, and pairing across them mixes @@ -252,26 +271,35 @@ On every merge, for each job name: ``` peak_MB = max(existing peak_MB, this week's max UsedMem_MB) # full precision, MB; a merge never lowers it peak_G = round(peak_MB / 1024, 1) # display only +peak_GB = peak_MB / 1024 rec_mem = round_up_to(mem_round_gb, - max(mem_floor_gb, margin_multiplier * peak_MB / 1024)) + max(mem_floor_gb, peak_GB * (1 + headroom_frac))) # headroom_frac default 0.30 ``` `rec_mem` is a **pure function of the stored `peak_MB`** and the `policy` values. It is **not** computed from this week's digest rows, and **not** from `peak_G`. +**The margin applies to a running max, not to a single observation.** `peak_MB` is the maximum +across *every* digest ever merged for that job name, so `rec_mem` is 30% above the **worst reading +ever seen** — not 30% above a typical week. That is what makes a margin this small defensible: the +number it multiplies has already absorbed every bad week in the row's history. + **`rec_mem` must never decrease while `peak_MB` is unchanged.** This is the property the whole table exists to guarantee, and it is the reason `peak_MB` is stored at all. Without it, a job whose -peak was 14963.82 MB one week (→ `peak_G 14.6`, `rec_mem 32G`) and which runs a smaller input the -next week (5000 MB) would keep `peak_G 14.6` but recompute `rec_mem` down to `12G` from this week's -smaller number — a recommendation below the row's own recorded peak, and an OOM on the next real -run. Recomputing from the stored `peak_MB` makes that arithmetically impossible. +peak was 14963.82 MB one week (→ `peak_G 14.6`, `rec_mem 20G`) and which runs a smaller input the +next week (5000 MB) would keep `peak_G 14.6` but recompute `rec_mem` down to `8G` — the floor — +from this week's smaller number, a recommendation less than half the row's own recorded peak and an +OOM on the next real run. Recomputing from the stored `peak_MB` makes that arithmetically +impossible. If a legacy table has no `peak_MB` column, add it and seed it as **`(peak_G + 0.05) * 1024`**. The `+ 0.05` is not padding, it is the rounding correction: `peak_G` is rounded to **nearest**, so a -bare `peak_G * 1024` can sit up to 51.2 MB *below* the true peak and migration would then lower a -recommendation — e.g. a true peak of 16394 MB displays as `peak_G 16.0`, whose true `rec_mem` is -36 G but whose bare-seeded `rec_mem` is 32 G, a 4 G drop in exactly the direction this column -exists to prevent. Seeding at the top of the rounding interval cannot go low. Say so in the report, +bare `peak_G * 1024` can sit up to 51.2 MB *below* the true peak. Whether that 51.2 MB changes +`rec_mem` depends on where it falls against the `mem_round_gb` step — but when it does cross a +boundary it costs a full 4 G, in exactly the direction this column exists to prevent. Worked case: +a true peak of 9462 MB displays as `peak_G 9.2`; its true `rec_mem` is 16 G, its bare-seeded +`rec_mem` is 12 G, and the corrected seed `(9.2 + 0.05) * 1024 = 9472 MB` gives 16 G again. Seeding +at the top of the rounding interval cannot go low. Say so in the report, and treat every such row's `rec_mem` as a lower bound until the next digest refreshes it. **The `IO-BOUND` flag: how it is set, and what it does** From eca7fb5804804a74e9f8377b0c298579df894b20 Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Tue, 4 Aug 2026 13:49:22 -0400 Subject: [PATCH 07/15] fix(session-tools): check scope at the point of use, define scope provenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A third execution test with a seeded submission log made the scope-join match for the first time, exposing a Critical defect and five smaller ones. C1 (CRITICAL): the scope column was recorded but never compared, so a subset measurement was presented as an actionable recommendation for a full run. A row measured at "5 of 100 units" carried no >= and read as usable, sizing a full 100-unit run from 5 units of evidence -- the exact under-provisioning the asymmetric rule exists to prevent. The root error was treating "we know what this measurement covered" as "this is safe to size a future run from"; those are different claims. The comparison also lived in the merge step, which is the one place it cannot be evaluated, since at merge time nobody knows what will be run next. Moved it to the point of use: slurm-sizing §2 gains "The scope check", which states the scope prominently and compares it to the intended run -- same scope is usable as-is, LARGER must not be sized down and is treated as a lower bound, SMALLER is safe but generous, undeterminable means ask. C2: scope provenance was undefined when a job name's rows disagree. Adopted: the scope that governs a row is the scope of the run that SET peak_MB, since the recommendation is derived from the peak. If that run has no declared scope the row stays >= regardless of what siblings declared, and a displaced scope is preserved in notes rather than discarded. C3: the reservation delta can go negative; now defined as under-provisioning relative to the job's own observed peak, never to be presented as a saving, absolute-valued or clamped. C4: row order was unspecified, so two merges of the same data could produce different files. Specified: sort by job_name ascending, every write. C5: the asymmetric rule had no first-ever-row branch. Stated as the same provenance rule with no history. C6: the scope prompt had no non-interactive behaviour. Unanswered names stay unknown, the merge completes, and a scope is never invented to fill the gap. rec_cpu, peak_MB monotonicity, headroom_frac, the duplicate gate and the enrichment procedure are untouched. --- .../skills/slurm-digest/SKILL.md | 62 +++++++++++++++---- .../skills/slurm-sizing/SKILL.md | 41 ++++++++++-- .../skills/slurm-sizing/reference/config.md | 20 +++++- 3 files changed, 104 insertions(+), 19 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index 14bd0c0..e1e77d2 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -288,16 +288,31 @@ for what each one means and the exact header to write if the table has to be cre or any other `policy` value, in config is choosing more OOM risk in exchange for queue priority, and should make that choice deliberately, in config, not by editing this file. - Then apply the asymmetric scope rule, comparing this week's `scope` (Step 5/6) against the one - recorded in the row's `scope` column: - - scope matches the recorded scope -> plain recommendation, no prefix - - scope is larger than recorded -> do NOT lower; scale by the known ratio or keep the prior - request and treat this run as a fresh measurement - - scope is smaller than recorded -> a smaller scope is not evidence about the larger one; do - NOT lower anything, treat it like unknown scope for recommendation purposes, and note the - scope it was actually measured at - - scope unknown (empty, or a `derived:` value not yet confirmed by a human) -> prefix `>=`; the - row may justify raising a request, never lowering one + **Then set `scope` and the `>=` prefix from the provenance of the peak — and understand what + this step can and cannot decide.** The merge records *what the measurement covered*. It cannot + decide whether the number is safe to size a future run from, because at merge time nobody knows + what the user will run next. That comparison happens at the point of use, in `slurm-sizing` §2. + So do not read an absent `>=` as "actionable" — it means "checkable". + + **The scope that governs a row is the scope of the run that set `peak_MB`.** A job name usually + has several rows in one digest with different jobids, and only one of them set the peak. The + recommendation is derived from the peak, so the peak's provenance is what governs it: + - **The peak-setting run has a declared scope** (matched in Step 5, or a `derived:` value a + human confirmed in Step 10) -> write that scope, no `>=` prefix. + - **The peak-setting run has no declared scope** -> `scope: unknown` (or the unconfirmed + `derived:` value) and prefix `>=`, **regardless of what sibling rows declared**. A scope + belonging to a run that did not set the peak says nothing about the peak. + - **`peak_MB` did not change this week** -> the governing scope did not change either. Leave + both alone. + - **First-ever row for this job name** -> there is no prior scope to compare against; take the + scope of the run that set its peak, or `unknown` with `>=` if that run has none. A new row is + not a special case, it is this same rule with no history. + - **A declared scope displaced by this rule is preserved, not discarded.** If a sibling run + declared a scope but did not set the peak, record it in `notes` (additively, per Step 9) — + e.g. `other-run scope: 5 of 100 units (jobid 7932158)`. It is real information about the job, + it is just not the provenance of this number. + + A `>=` row may justify raising a request, never lowering one. 9. **CPU.** **First pick the row, then apply the formula.** A job name usually has **several rows in one digest** (nine runs of the same name is ordinary), and their `CPUPct` values differ @@ -360,6 +375,15 @@ for what each one means and the exact header to write if the table has to be cre the user is asked about, which is the only mechanism that turns `>=` rows into real recommendations, so a mis-ordered list has a real cost. Names left unanswered stay `unknown` and carry forward. + + **If no human is available to answer, that is a normal outcome, not a failure.** When this runs + unattended, or the user declines to answer, or the session ends before they do: leave every + unanswered name at `scope: unknown` with its `>=` prefix, **complete the merge anyway**, and + say in the report how many names went unanswered. Do **not** block the merge waiting for a + reply, and above all do **not** invent, infer or guess a scope to fill the gap — a fabricated + scope removes a `>=` and turns a lower bound into a confident wrong answer, which is strictly + worse than the `unknown` it replaced. The peaks still merged; only the scope is missing, and it + can be supplied on any later run. **Show derived evidence alongside the prompt.** For any unmatched name Step 6 enriched with a `derived:` scope, display that derived submit line next to it so the human can confirm or correct it. **Both confirmation and correction promote it identically:** whether the human @@ -371,7 +395,10 @@ for what each one means and the exact header to write if the table has to be cre 11. **Persist, then archive.** This is the point the on-disk table changes: write the fully merged and recomputed table (Steps 7-10 applied) back to the configured `table` path now, - preserving the nine-column header. Only after that write succeeds, write the raw pasted digest + preserving the nine-column header and **sorting all data rows by `job_name` ascending**. The + sort is not cosmetic: an unspecified order lets two merges of the same data produce different + files, which makes every diff noisy and hides real changes during review. Ascending `job_name` + is stable as rows are added. Only after that write succeeds, write the raw pasted digest to `/YYYY-MM-DD.tsv` under the week-ending date from Step 1 (creating the configured `archive` directory with `mkdir -p` if it does not exist) — the duplicate check already happened in Step 1, so this write should never collide with an existing file under normal @@ -390,10 +417,21 @@ for what each one means and the exact header to write if the table has to be cre Report it split, never as one number: - **Actionable subtotal** — summed over rows whose `rec_mem` has **no** `>=` prefix. This is - reservation that can actually be given back now. + reservation that could be given back, subject to the scope check `slurm-sizing` §2 applies + when the number is actually used. - **Pending-scope subtotal** — summed over rows whose `rec_mem` **does** carry `>=`. This is not actionable; it is what capturing scope would unlock. + **A negative delta is meaningful, not an error — report it as what it is.** A row contributes + negatively when `rec_mem_GB > ReqMem_GB`, i.e. the job's own observed peak (plus headroom) is + larger than what it asked for: applying the recommendation would **increase** that row's + reservation. That is a real signal of **under-provisioning relative to its own measured peak**, + and often means the job has been running close to or into its limit. Never present a negative + subtotal as a saving, never take its absolute value, and never silently clamp it to zero. If a + subtotal is negative overall, say so in those terms — "applying these would add N GB, because + these jobs are asking for less than they have been measured using" — and name the rows driving + it, since each one is worth a look on its own. + Merging the two into a single figure would contradict this step's own statement that `>=` rows are not actionable for sizing down, and the split is the more useful number anyway. Say the units (GB) and be explicit that `>=` rows are not actionable for sizing down. diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index 79885c0..e85cd7e 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -63,16 +63,45 @@ nine columns (`job_name`, `peak_MB`, `peak_G`, `n`, `scope`, `last_seen`, `rec_m [reference/config.md](reference/config.md) → "Table file format". - **Table file does not exist:** go to **Bootstrap** (§6). -- **Job name is present in the table:** use that row's `rec_mem` / `rec_cpu` and state which row - you used (e.g. "using the `align_star` row: rec_mem=32G, rec_cpu=8"). Quote `n` too — `n = 1` is - a single observation, not a characterisation. - - **The value carries a `>=` prefix:** that's a lower bound from a run whose scope was never - recorded, so the true peak could be higher. It may justify *raising* the request above the - table value; it must never justify *lowering* the request below it. +- **Job name is present in the table:** state which row you used, its `rec_mem` / `rec_cpu`, its + `n` (`n = 1` is a single observation, not a characterisation) — **and its `scope`**. Then apply + the scope check below **before** treating any number as usable. - **Job name is absent from the table:** say so explicitly, then either ask what scope to expect or start small and measure. Never fall back to a remembered/habitual number ("jobs like this usually need 64G") — an absent row is a reason to measure, not a reason to guess. +### The scope check — do this every time, before quoting a number as usable + +**A recorded scope makes a row *checkable*. It does not make it *actionable*.** Those are +different claims, and conflating them is how this system produces a confidently wrong answer. +`scope` records what the measurement covered; whether the number is safe for *this* run depends on +what you are about to submit, which only you know — the merge step could not have known it. + +State the row's `scope` prominently, then compare it to the run about to be submitted: + +- **No declared scope (`rec_mem` carries `>=`, or `scope` is empty / `unknown` / a `derived:` value + a human has not confirmed):** a lower bound from a run whose workload was never recorded, so the + true peak could be higher. It may justify *raising* the request above the table value; it must + never justify *lowering* the request below it. +- **Declared scope, and the intended run is the SAME scope:** the number is usable as-is. Say which + scope you matched (e.g. "the row was measured at `1565 tiles (FULL)`, which is what you're about + to run"). +- **Declared scope, but the intended run is LARGER:** **do not use this number to size down.** + Treat it exactly as a lower bound — `>=` in effect — and say why: the measurement covered less + work than the run being sized, so it is evidence about a smaller job, not about this one. Either + scale by the known ratio if the relationship is genuinely linear and you say so, or keep the + larger request and treat this run as a fresh measurement. A row reading `36G` measured at + "5 of 100 units" is **not** a 36G recommendation for all 100. +- **Declared scope, but the intended run is SMALLER:** a smaller run is not evidence for a bigger + one, and this row *is* the bigger one — the number is safe but likely generous. You may use it; + note that it was measured at a larger scope, and that measuring the smaller run would tighten it. +- **Cannot determine the intended run's scope, or cannot tell how it compares:** **ask.** Do not + assume they match. "Same scope" is a claim about the workload, not a default. + +This check is the reason `scope` exists in the table at all. Skipping it turns a subset measurement +into a full-run recommendation, which is the specific under-provisioning failure the column was +added to prevent. + ## 3. Never size from raw `sacct` MaxRSS on an I/O-heavy job `MaxRSS` on a job that reads or writes a lot of data includes kernel page cache charged to that diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index e9228c2..8d92a18 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -237,6 +237,11 @@ Measured memory and CPU usage per job name, merged from weekly usage digests by |---|---|---|---|---|---|---|---|---| ``` +**Row order is part of the format: data rows are sorted by `job_name` ascending.** Every write +re-sorts. An unspecified order lets two merges of the same data produce different files, making +diffs noisy and hiding real changes during review; ascending `job_name` is stable as rows are +added. + **How to read a row** - **`job_name`** — the Slurm `JobName` exactly as it appears in the digest. This is the lookup key; @@ -257,9 +262,22 @@ Measured memory and CPU usage per job name, merged from weekly usage digests by "12 tasks x 1565 tiles (FULL)"). Empty or `unknown` means the peak is only a **lower bound** — nothing is known about what it was processing. A `derived: ` value is evidence recovered from `sacct`, not a human-declared scope, and still counts as unknown until confirmed. + + **The scope that governs a row is the scope of the run that set `peak_MB`** — not any sibling + run's. A job name usually has several runs in a digest and only one set the peak; the + recommendation is derived from the peak, so the peak's provenance is what governs it. If the + peak-setting run has no declared scope the row stays `>=`, whatever other runs of the same name + declared, and a displaced scope is preserved in `notes` rather than discarded. + + **A recorded scope makes the row *checkable*, not automatically *actionable*.** It says what the + measurement covered; it does not say the number is safe for the next run. Whoever uses the number + must compare this scope against the run they are about to submit — see `slurm-sizing` §2, "The + scope check". A `36G` row measured at "5 of 100 units" is not a `36G` recommendation for 100. - **`last_seen`** — the week-ending date of the most recent digest that contained this job. - **`rec_mem`** — the recommended `--mem`, computed **from `peak_MB`**. A `>=` prefix means the - scope was unknown: the value may justify *raising* a request, never *lowering* one. + peak-setting run had no declared scope: the value may justify *raising* a request, never + *lowering* one. **No prefix does not mean "use it unconditionally"** — it means the scope is + known and must be checked against the intended run. - **`rec_cpu`** — the recommended `--cpus-per-task`, capped at the job's own `ReqCPU`. - **`notes`** — free text, **additive**: appending a note must preserve what is already there. `IO-BOUND` in this column is a load-bearing flag, not a comment — see below. From 1478fd131c08f8b474684149bd5fcc044b6d3b7e Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Tue, 4 Aug 2026 14:50:36 -0400 Subject: [PATCH 08/15] docs(session-tools): answer the larger-scope case, mark hard stops, explain pinned rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth execution test -- the first to run slurm-sizing itself, against a table seeded to force every branch. It CONFIRMED the C1 scope-check fix: a row recorded at "5 of 100 samples (SUBSET)", asked to size a 100-sample run, was correctly treated as a lower bound instead of certified at its table value. Exact-match, unknown-scope, IO-BOUND and absent-row branches all behaved correctly, the last two for the first time. Three documentation findings. - F1: "do not size down" never said what to request INSTEAD, so a correct refusal left the researcher with no number and moved the guess somewhere invisible. §2's scope check now states plainly that the table cannot answer this -- it holds a measurement of a different-sized run -- and gives three ordered routes: scale by a known parameter and LABEL THE RESULT AN ESTIMATE, or start from a larger run that previously succeeded, or measure first and merge that digest. Plus: never present the table's number as if it covered the larger run, not even silently as a floor, and record the scope at submission either way. - F2: the ask-the-user points were not self-enforcing -- an unattended agent proceeded past two of them. Both skills gain a "Hard stops" section: a HARD STOP is a halt, not a suggestion, and running unattended is not permission to continue. Marked at the undeterminable-scope branch, the missing date, the already-archived digest, the first multi-user encounter, and every bootstrap question. Step 10's scope prompt is documented as deliberately NOT a hard stop, since its unanswered outcome is the conservative one. - F3: an IO-BOUND row's rec_mem does not satisfy the formula, and nothing said it shouldn't -- the tester reasonably reported it as a data-integrity gap. Both files now state that a pinned row's rec_mem is hand-set from in-process evidence and deliberately NOT derived from peak_MB, that a mismatch is expected, and that "correcting" it discards the only evidence the row has. PR body updated: four execution tests and what each found, why only a seeded-fixture test could reach the Critical, and an honest unverified list (wrapper-script triggering, SubmitLine retention, and the absence of a scaling model when a run exceeds its measured scope). --- .../skills/slurm-digest/SKILL.md | 33 ++++++-- .../skills/slurm-sizing/SKILL.md | 82 +++++++++++++++++-- .../skills/slurm-sizing/reference/config.md | 24 ++++-- 3 files changed, 118 insertions(+), 21 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index e1e77d2..dd49b1c 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -46,21 +46,39 @@ Merge the Slurm digest pasted below into the sizing table at the configured `tab [../slurm-sizing/reference/config.md](../slurm-sizing/reference/config.md) → "Table file format" for what each one means and the exact header to write if the table has to be created. +## Hard stops + +Several points below say to ask the user. Those marked **HARD STOP** are halts, not suggestions: +stop, report what you need, and **do not proceed on a default**. **Running unattended is not +permission to continue** — if there is no human to answer, stop and report that the merge did not +run, rather than choosing a value. Proceeding silently past one of these is how an unsafe number +enters the table; everything else here is built so the *conservative* outcome is the automatic one, +and a hard stop marks the places where there is no conservative outcome to fall back to. + +The hard stops in this skill are: **a missing week-ending date, and an already-archived digest +(Step 1)**; **the first multi-user digest encounter (Step 3)**; and **every question in Bootstrap** +(`digest_cluster`, `digest_user`, decline — see the config contract). + +**Step 10's scope prompt is deliberately NOT a hard stop.** Its unanswered outcome is a documented +conservative one — `scope: unknown` with a `>=` prefix — and the merge completes. The distinction +throughout is whether continuing requires *assuming* something unverified. + ## Procedure 1. **Establish the week-ending date, then refuse duplicates (precondition — check this FIRST, before parsing or merging anything).** The digest's date is **a required argument**, supplied alongside the pasted table (e.g. `/session-tools:slurm-digest 2026-08-01 `). The digest body itself contains no date column, so it cannot be recovered from the paste. - **If the date is missing, ASK for it — never assume today's date.** Defaulting to today lets the - same digest pasted on two different days archive under two filenames, pass this duplicate check - both times, and double `n` — exactly the corruption this step exists to prevent. + **If the date is missing: HARD STOP — ask for it, and never assume today's date.** Defaulting to + today lets the same digest pasted on two different days archive under two filenames, pass this + duplicate check both times, and double `n` — exactly the corruption this step exists to prevent. With the date in hand, check whether `/YYYY-MM-DD.tsv` already exists for it (`Read` it; a "no such file" error is the not-archived case), where `` is the configured `archive` path (default `~/.claude/slurm-sizing/digests`). - **If it exists, STOP here: change nothing** — do not parse, join, merge, or write anything — - report that the digest is already archived for that date, and ask whether to force. Re-merging - an already-counted digest would inflate `n` (nine `score` runs would read as eighteen), and `n` + **If it exists: HARD STOP — change nothing** — do not parse, join, merge, or write anything — + report that the digest is already archived for that date, and ask whether to force. A forced + re-merge requires an explicit human instruction; unattended, stop and report. Re-merging + an already-counted digest would inflate `n` (nine runs of a name would read as eighteen), and `n` is the signal that says whether a recommendation is trustworthy. Only proceed past this step if no file exists for that date, or the user has explicitly instructed you to force a re-merge. @@ -90,7 +108,8 @@ for what each one means and the exact header to write if the table has to be cre with no error anywhere, so this filter runs before anything downstream sees a row. Precisely what to do when other users' rows appear — ask once, then remember: - **First time** (`multi_user_digest` is unset or `false` in config) **and other users are - present:** HALT before merging anything. Report which other users are present and how many + present:** **HARD STOP** — halt before merging anything, and halt even when running + unattended; do not filter on a default. Report which other users are present and how many rows each has, and ask whether this is a shared digest that should be filtered to `digest_user`. - **On confirmation:** write `"multi_user_digest": true` into the config, then proceed, diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index e85cd7e..614f138 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -88,20 +88,67 @@ State the row's `scope` prominently, then compare it to the run about to be subm to run"). - **Declared scope, but the intended run is LARGER:** **do not use this number to size down.** Treat it exactly as a lower bound — `>=` in effect — and say why: the measurement covered less - work than the run being sized, so it is evidence about a smaller job, not about this one. Either - scale by the known ratio if the relationship is genuinely linear and you say so, or keep the - larger request and treat this run as a fresh measurement. A row reading `36G` measured at - "5 of 100 units" is **not** a 36G recommendation for all 100. + work than the run being sized, so it is evidence about a smaller job, not about this one. A row + reading `36G` measured at "5 of 100 units" is **not** a 36G recommendation for all 100. + + **Say plainly that the table alone cannot answer this** — it holds a measurement of a + different-sized run, and no amount of reading it turns that into a number for this one. Then + produce an actual answer, taking the first of these three routes that applies. Do not stop at the + refusal: leaving the researcher with no number is not a safe outcome, it just moves the guess + somewhere nobody can see it. + + 1. **A scaling parameter is known** (samples, tiles, cells, perturbations, reads — whatever the + scope is counted in): scale the measured peak by the ratio of intended to measured units, + apply the normal headroom, and **label the result an estimate, not a measurement**. Show the + arithmetic and the assumption — e.g. "measured 27.5 G at 5 of 100 samples; 20x the work, so + ~550 G plus headroom — an estimate that assumes memory scales linearly with samples, which it + may not." If you have reason to think it is not linear, say which way you expect it to be + wrong. + 2. **No scaling parameter, but a larger run of this job has succeeded before:** start from that + run's request. A request that actually completed is evidence; the table's smaller-scope row is + not evidence about this size. + 3. **Neither:** measure first. Run a small or intermediate case, merge the resulting digest with + `/session-tools:slurm-digest`, and size the big run from the row that produces. Committing a + large reservation on an unscaled subset measurement is exactly the guess this system exists to + replace. + + **Never present the table's number as if it covered the larger run** — not as the recommendation, + and not silently as a floor without saying that a floor is all it is. + + Whichever route you take, **record the scope at submission time per §4.** That is the whole + mechanism by which the next person asking this question gets a better answer than you did. - **Declared scope, but the intended run is SMALLER:** a smaller run is not evidence for a bigger one, and this row *is* the bigger one — the number is safe but likely generous. You may use it; note that it was measured at a larger scope, and that measuring the smaller run would tighten it. -- **Cannot determine the intended run's scope, or cannot tell how it compares:** **ask.** Do not - assume they match. "Same scope" is a claim about the workload, not a default. +- **Cannot determine the intended run's scope, or cannot tell how it compares: HARD STOP — ask.** + This is a halt, not a suggestion (see "Hard stops" below). Do not assume they match. "Same scope" + is a claim about the workload, not a default. This check is the reason `scope` exists in the table at all. Skipping it turns a subset measurement into a full-run recommendation, which is the specific under-provisioning failure the column was added to prevent. +### Hard stops + +A **HARD STOP** in this skill means: stop, report what you need, and **do not proceed on a +default**. It is not a prompt you may skip when no one answers. + +- **Running unattended is not permission to continue.** If there is no human to answer, **stop and + report** — state the question, what you would have needed, and that no sizing was produced. An + agent operating without a user must not pick a value and carry on. +- **Why these hold the whole mechanism together:** proceeding silently past an unanswered scope + question is precisely how an unsafe number gets used. Every other safeguard here — `>=`, the + running max, the scope check — is designed so that the *conservative* answer is the automatic + one. A hard stop marks the places where there is no conservative answer to fall back to, only a + guess. +- The hard stops in this skill are: **the undeterminable-scope branch above**, and **every question + in Bootstrap (§6)** — `digest_cluster`, `digest_user`, and the decline option. Never invent a + cluster name, an account name, or a scope. +- **Not every question is a hard stop.** Where a documented conservative fallback exists and is + applied, taking it is correct — e.g. `slurm-digest`'s Step 10 scope prompt, whose unanswered + outcome is `unknown` plus a `>=` prefix, and whose merge completes. The distinction is whether + continuing requires *assuming* something unverified. + ## 3. Never size from raw `sacct` MaxRSS on an I/O-heavy job `MaxRSS` on a job that reads or writes a lot of data includes kernel page cache charged to that @@ -115,11 +162,22 @@ and don't recompute an `IO-BOUND` row from `sacct`. 1. Get **in-process** evidence of the real peak RSS — `resource.getrusage(...).ru_maxrss` inside the job, a `psutil`-style RSS sampler, or a memory profiler. A digest or `sacct` number can never justify the flag: distrusting those numbers is the entire point of it. -2. Set the row's `peak_MB` to that measured peak; `peak_G` and `rec_mem` follow from it by the - formulas in [reference/config.md](reference/config.md). -3. **Append** `IO-BOUND` to `notes`, preserving whatever text is already there, and record where +2. Set the row's `peak_MB` to that measured peak, and `peak_G` to its display rounding. +3. Set `rec_mem` **by hand**, from the same in-process evidence — the measured demand plus whatever + headroom that evidence justifies. See the next paragraph: this is deliberately not the formula. +4. **Append** `IO-BOUND` to `notes`, preserving whatever text is already there, and record where the measurement came from — e.g. `IO-BOUND (getrusage peak 3.2G, 2026-08-03); CPUPct 45`. +**An `IO-BOUND` row's `rec_mem` will not reconcile with the formula, and that is correct.** On every +other row `rec_mem` is `peak_GB × (1 + headroom_frac)`, floored and rounded up. On a pinned row it +is **hand-set from in-process evidence and deliberately NOT derived from `peak_MB`** — the two +numbers can come from different measurements of different things, so applying the formula to the +row's `peak_MB` may well give a different answer (a small pinned `peak_MB` can sit beside a larger +hand-set `rec_mem`, or the reverse). **A mismatch there is expected and must not be "corrected".** +Recomputing a pinned row from its `peak_MB` silently discards the in-process evidence that is the +only reason the row exists. If a pinned row looks inconsistent, that is the flag working, not a +data-integrity bug. + Removing the flag requires the same class of evidence that set it — never a digest, never `sacct`. ## 4. After submitting — log what it actually ran @@ -151,6 +209,12 @@ attaching to another cluster's job. Full file formats, the literal table header, and the rationale are in [reference/config.md](reference/config.md) → "Bootstrap". The branches, so none gets skipped: +**Every question in this section is a HARD STOP** (see §2, "Hard stops"): `digest_cluster`, +`digest_user`, and the decline option all require an answer from the user. Running unattended is not +permission to pick one — stop and report that bootstrap could not complete. A guessed cluster name +makes the skill inert or wrong forever, and a guessed account name filters out every one of the +user's own rows; neither failure announces itself. + - **Config missing:** check first for a pre-plugin layout — probe with `test`, since one of the three is a directory: `test -f ~/.claude/slurm-sizing.md`, `test -f ~/.claude/slurm-jobs.tsv`, `test -d ~/.claude/slurm-digests`. If any exist, offer to point the new config at them instead of diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index 8d92a18..04da0bc 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -133,7 +133,8 @@ input; guessing whose rows to keep is not. Precisely what to do when other users' rows appear — **ask once, then remember**: -- **First time:** HALT before merging anything. Report which other users are present and how many +- **First time:** **HARD STOP** — halt before merging anything, and halt even when running + unattended; do not filter on a default. Report which other users are present and how many rows each has, and ask whether this is a shared digest that should be filtered to `digest_user`. - **On confirmation:** write `"multi_user_digest": true` into the config, then proceed, filtering to `digest_user` and reporting the filtered-out count each week thereafter. Do not halt again. @@ -173,7 +174,11 @@ directories as needed when bootstrapping. The config file's own location is fixe ## Bootstrap (first run, nothing exists yet) -**Config missing:** +**Config missing:** every question in this branch is a **HARD STOP** — `digest_cluster`, +`digest_user`, and the decline option all require an answer from the user. **Running unattended is +not permission to pick one**: stop and report that bootstrap could not complete. A guessed cluster +name makes `slurm-sizing` inert or wrong forever; a guessed account name filters out every one of +the user's own rows. Neither failure announces itself. 1. **Check for a pre-plugin layout first** — two files and one **directory**, where an early hand-rolled version kept its data. Probe all three with `test`, which answers for a directory as @@ -332,13 +337,22 @@ from a real in-process measurement instead. 1. Obtain **in-process** evidence of the job's real peak RSS: `resource.getrusage(...).ru_maxrss` inside the job, a `psutil`-style RSS sampler, or a memory profiler. `sacct`/digest numbers are exactly what the flag exists to distrust, so they can never justify setting it. -2. Set the row's `peak_MB` to that measured peak, and let `peak_G` / `rec_mem` follow from it by - the formulas above. -3. **Append** `IO-BOUND` to the `notes` column, keeping any existing note text, and say where the +2. Set the row's `peak_MB` to that measured peak, and `peak_G` to its display rounding. +3. Set `rec_mem` **by hand**, from the same in-process evidence — see the reconciliation note below. +4. **Append** `IO-BOUND` to the `notes` column, keeping any existing note text, and say where the measurement came from — e.g. `IO-BOUND (getrusage peak 3.2G, 2026-08-03); CPUPct 45`. *What it does* — `slurm-digest` leaves an `IO-BOUND` row alone: its peak is not merged, its `rec_mem`/`rec_cpu` are not recomputed, and its `notes` are not rewritten. `slurm-sizing` trusts the pinned row over any `sacct`-derived figure for the same job. +**A pinned row's `rec_mem` does not reconcile with the formula, by design.** Every other row +satisfies `rec_mem = round_up_to(mem_round_gb, max(mem_floor_gb, peak_GB * (1 + headroom_frac)))`. +An `IO-BOUND` row does **not**: its `rec_mem` is **hand-set from in-process evidence and is +deliberately not derived from `peak_MB`**, so recomputing the formula against that row's `peak_MB` +can give a different number in either direction. **That mismatch is expected and must not be +"corrected."** It is not a data-integrity gap, and anything that "fixes" it silently throws away +the in-process measurement that is the row's entire justification. Validate a pinned row by its +recorded evidence, never by re-deriving it. + *Removing it* requires the same class of evidence that set it — never a digest, never `sacct`. From a07d46667eaa14e63259dabf36ee7290481f0ebd Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Tue, 4 Aug 2026 15:50:06 -0400 Subject: [PATCH 09/15] fix(session-tools): cluster identity becomes ClusterName@SlurmctldHost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live install check on a second machine found that the (cluster, jobid) join key does not actually separate clusters. Two hosts at one institute report the SAME ClusterName while having different controllers, different accounting databases and independent job-ID spaces: the same job number named two different jobs, submitted a year apart, depending on which host you asked. Because ~/.claude is shared NFS storage, one submission log receives rows from both, tagged identically -- precisely the corruption the key was introduced to prevent. A Slurm ClusterName is a locally chosen label with no uniqueness guarantee across the clusters one person can reach, so a bare name is not a safe key. RULING implemented: cluster identity is now the composed string @, both fields read from `scontrol show config`. It is what digest_cluster holds, what the log's cluster column carries, and what the local-vs-digest gate compares. Two clusters with independent job-ID spaces necessarily have distinct controllers, so the suffix separates them; where ClusterName is already unique it changes nothing but makes the identity explicit. Applied across all three files -- the config key, the shared determination procedure, the bootstrap step, the join-key discussion, slurm-sizing's §1 gate / §4 log-append / §5 rationale, and slurm-digest's lookup step and enrichment guard -- and checked against each other. Bootstrap derives and DISPLAYS the composed string, showing both source fields so the user can tell which machine they are on. Legacy bare names are treated as UNVERIFIED, not upgraded: such a value may belong to any cluster with that name, so it cannot satisfy a scope match (the row stays >=), and it is never rewritten into a composed identity -- no evidence survives about which cluster wrote it, so either would manufacture provenance. The count is reported so a drop in matches reads as the transition rather than a malfunction. Also: sacctmgr is no longer instructed anywhere now that scontrol supplies both fields, so Bash(sacctmgr *) is removed from BOTH skills. Per the lesson from the last permission removal, both bodies were then re-audited command by command rather than only the file being edited. Shipped text describes the finding by shape ("two clusters reachable from one shared home reported the same ClusterName") and uses invented identities (alpha@ctl-1, alpha@ctl-2); no real hostnames appear. --- .../skills/slurm-digest/SKILL.md | 66 +++++++--- .../skills/slurm-sizing/SKILL.md | 56 ++++++-- .../skills/slurm-sizing/reference/config.md | 121 +++++++++++++----- 3 files changed, 176 insertions(+), 67 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index dd49b1c..64c3783 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -6,7 +6,7 @@ description: >- week-ending date it covers, then run it explicitly; this skill does not trigger on its own the way slurm-sizing does. user-invocable: true -allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(sacct *), Bash(sacctmgr *), Bash(scontrol *) +allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(sacct *), Bash(scontrol *) --- # Slurm Digest Merge @@ -132,12 +132,22 @@ throughout is whether continuing requires *assuming* something unverified. configured `log` path (default `~/.claude/slurm-sizing/jobs.tsv`) is tab-separated and begins with `#`-prefixed comment lines before its header row `cluster jobid submitted job_name scope cwd`. - **Digests come from the configured `digest_cluster` only; the log's rows carry their own - `cluster` value, which may name a different cluster for jobs submitted elsewhere.** - `digest_cluster` and other clusters are separate Slurm ID namespaces — the same `jobid` number - can label unrelated jobs on each one — so a bare `jobid` lookup is a correctness bug: it will - silently splice an unrelated cluster's job onto this digest's scope and produce a confidently - wrong recommendation. Joining on `jobid` alone is never acceptable, even as a shortcut. + **The `cluster` column holds a composed identity, `@`** (e.g. + `alpha@ctl-1`), and so does `digest_cluster`. Digests come from the configured `digest_cluster` + only; the log's rows carry their own identity, which may name a different cluster for jobs + submitted elsewhere — `~/.claude` is often shared/NFS storage, so one log routinely receives + rows from every cluster its owner touches. Separate clusters are separate Slurm ID namespaces — + the same `jobid` number can label unrelated jobs on each — so a bare `jobid` lookup is a + correctness bug: it will silently splice an unrelated cluster's job onto this digest's scope and + produce a confidently wrong recommendation. Joining on `jobid` alone is never acceptable, even + as a shortcut. + + **And matching on a bare `ClusterName` is not sufficient either.** A `ClusterName` is a locally + chosen label with no uniqueness guarantee: **two clusters reachable from one shared home were + found reporting the same `ClusterName` while having separate controllers, separate accounting + databases and independent job-ID spaces.** That is why the identity carries the controller host — + two clusters with independent job-ID spaces necessarily have distinct controllers. Compare the + whole composed string, never just the part before the `@`. **Do this with a real shell command, not by reasoning over `Read` output**, so the composite key and the tab delimiter are actually enforced. One `awk` lookup does all of it — comment @@ -161,11 +171,21 @@ throughout is whether continuing requires *assuming* something unverified. field and needs both inputs pre-sorted, while this log is append-ordered. - **`grep -v '^#'`** — the comment header is not data. + **Legacy rows carrying a bare cluster name are unverified and must not match.** Rows written + before the composed identity existed hold just a `ClusterName`, which may belong to any cluster + with that name. The `$1 == c` test already excludes them, since `c` contains an `@` — that is + the correct outcome, not a bug to work around. **Do not relax the comparison to match on the + `ClusterName` part, and do not rewrite such rows**: no evidence survives about which cluster + wrote them, so either would manufacture provenance. Count them and report how many log rows were + skipped as unverified-legacy, so a drop in matches is visibly the transition rather than a + malfunction. New rows carry the composed identity, so this fades on its own. + A digest row with no output line gets `scope: unknown`. **If zero rows match, say so prominently** — a silently failing lookup makes every recommendation a lower bound and the system quietly stops working. Because `slurm-sizing` refuses to log on any cluster other than `digest_cluster`, a populated log should contain `digest_cluster` rows; a zero match against a - non-empty log means the lookup is broken, not that there was nothing to find. + non-empty log means the lookup is broken (or every row is unverified legacy, per above), not + that there was nothing to find. 6. **Enrich unmatched rows from `sacct` — a conditional recovery procedure, not part of the lookup.** For every digest row that did NOT match a submission-log entry in Step 5, attempt to @@ -183,19 +203,27 @@ throughout is whether continuing requires *assuming* something unverified. retention gap). Keep the original suffixed ID on the digest row for reporting; normalise only the lookup key. - **Cluster guard, checked first.** `sacct` only sees the LOCAL cluster's accounting - database — **verified in practice**: a job ID from the digest's cluster does not resolve - via `sacct` run against a different cluster's accounting database (`sacct -L -j ` - returns nothing for it there), and `sacctmgr -n -P list cluster format=Cluster` lists only - the local cluster's name. Determine the local cluster with the **shared procedure** in + database — **verified in practice**: a job ID from another cluster does not resolve via + `sacct` run against this one's accounting database (`sacct -L -j ` returns nothing for + it there). Determine the **local cluster identity** with the **shared procedure** in [../slurm-sizing/reference/config.md](../slurm-sizing/reference/config.md) → "Determining the - local cluster" (`sacctmgr -n -P list cluster format=Cluster`, falling back to - `scontrol show config | grep ClusterName`, taking the value after the `=`) — **never - `hostname`**, which returns a node name, not a cluster name, and would therefore fail this + local cluster identity": + + ```bash + scontrol show config | grep -E 'ClusterName|SlurmctldHost\[0\]' + ``` + + Take the value after each `=`, trimmed, and compose `@` — **never + `hostname`**, which returns a node name, not a cluster identity, and would therefore fail this comparison on essentially every site while reporting the result as a deliberate skip. Compare - it to the digest's cluster (`digest_cluster` from config). If they differ, **skip enrichment - entirely** for the whole digest — do not query `sacct` — and say so plainly in the report - (Step 12). If the local cluster is undeterminable, treat that the same way: skip and say so. - Never fabricate a scope to compensate. + the **whole composed string** to `digest_cluster` from config; **comparing only the + `ClusterName` part is not sufficient** — two clusters reachable from one shared home were + found reporting the same `ClusterName` with independent job-ID spaces, so a name-only match + would query the wrong accounting database and silently attach another cluster's submit line as + this job's scope. If they differ, **skip enrichment entirely** for the whole digest — do not + query `sacct` — and say so plainly in the report (Step 12). If either field is missing, the + local identity is undeterminable: skip and say so, and do **not** fall back to the bare + `ClusterName`. Never fabricate a scope to compensate. - **Batch query, one field per call — never a combined multi-field query.** Query once for every unmatched row, not per row, but query `SubmitLine` and `WorkDir` in **separate** calls: `sacct -j --format=JobID,SubmitLine --parsable2 --noheader` and, only diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index 614f138..d2a7c96 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -12,7 +12,7 @@ description: >- about Slurm resource sizing. SKIP only when no cluster job is involved at any remove, or when the user has turned this system off (`"enabled": false` in its config). user-invocable: true -allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(sacctmgr *), Bash(scontrol *), Bash(grep *) +allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(scontrol *), Bash(grep *) --- # Slurm Job Sizing @@ -39,21 +39,31 @@ data paths inside it are configurable). ## 1. Cluster gate — check this before anything else -1. Read `digest_cluster` from the config. -2. Determine the local cluster using the shared procedure in - [reference/config.md](reference/config.md) → "Determining the local cluster": - `sacctmgr -n -P list cluster format=Cluster`, falling back to - `scontrol show config | grep ClusterName` (take the value after the `=`, trimmed). **Never use +1. Read `digest_cluster` from the config. It holds a **composed identity**, + `@` (e.g. `alpha@ctl-1`) — not a bare cluster name. +2. Determine the **local cluster identity** using the shared procedure in + [reference/config.md](reference/config.md) → "Determining the local cluster identity": + + ```bash + scontrol show config | grep -E 'ClusterName|SlurmctldHost\[0\]' + ``` + + Take the value after each `=`, trimmed, and compose `@`. **Never use `hostname`** — it names a node, not a cluster. - - **Both commands fail or return nothing:** the local cluster is undeterminable. Say so - explicitly and STOP — do not consult the table, do not append to the log. "Unknown" is never - treated as a match. -3. Compare the two values — an exact string match. + - **Either field is missing, or the command fails:** the local identity is undeterminable. Say + so explicitly and STOP — do not consult the table, do not append to the log. **Do not fall + back to the bare `ClusterName`** — that is the unsafe key this composition replaces. + "Unknown" is never treated as a match. +3. Compare the two composed identities — an exact string match. - **They match:** proceed to §2. - - **They differ:** say so explicitly (name both clusters) and STOP. Do not consult the table + - **They differ:** say so explicitly (name both identities) and STOP. Do not consult the table and do not append to the log. This system is inert on any cluster other than the one the digest was built from — job IDs collide across clusters (see §5), so numbers from the wrong cluster are worse than no numbers at all. + - **`digest_cluster` is a bare name with no `@`** (written before this rule existed): treat it + as **unverified** — it may name any cluster carrying that `ClusterName`. Say so and STOP, + asking the user to re-derive it through Bootstrap (§6). Do not silently upgrade it by + appending the local controller host: that asserts provenance nobody recorded. ## 2. Before sizing any job — read the table first @@ -189,21 +199,39 @@ cluster jobid submitted job_name scope cwd ``` - **Log file does not exist:** go to **Bootstrap** (§6), then append the row. -- `cluster` must be the exact `digest_cluster` string from the config — the digest merge filters - log rows by exact match on this column, so a variant spelling makes the row invisible. +- `cluster` must be the exact `digest_cluster` string from the config — the full composed + `@` identity, never a bare cluster name. The digest merge filters log + rows by exact match on this column, so a variant spelling makes the row invisible, and a bare + name makes it **unverifiable** (see §5). - `scope` is a short, concrete description of the workload this run did — e.g. "12 tasks x 1565 tiles (FULL)", "5 of 100 perturbations" — not a repeat of the job name. - A peak logged with no `scope` is only ever a lower bound: once the job has finished, `sacct` has no way to recover what it actually processed. Write the scope now, while it's known, even before the peak usage is known. -## 5. Why `cluster` is the first column +## 5. Why `cluster` is the first column, and why it is a composed identity Job IDs are not unique across clusters — two different clusters can each have a job `123456`. The join key for this whole system is the pair `(cluster, jobid)`, never `jobid` alone. Putting `cluster` first keeps that join key visible and stops one cluster's workload scope from silently attaching to another cluster's job. +**But a bare cluster name is not a safe value for that column.** A Slurm `ClusterName` is a locally +chosen label with no guarantee of uniqueness across the clusters one person can reach. **Two +clusters reachable from one shared home directory were found reporting the same `ClusterName`, +with separate controllers, separate accounting databases and independent job-ID spaces** — the same +job number naming two unrelated jobs depending on which host you asked. Because `~/.claude` is +often on shared/NFS storage, one log receives rows from both, tagged identically. That is the exact +corruption this key exists to prevent, so the key must be +**`@` + jobid**. Two clusters with independent job-ID spaces +necessarily have distinct controllers, so the suffix separates them; where `ClusterName` is already +unique it changes nothing. + +**Legacy rows written with a bare name are unverified.** They may have come from any cluster with +that name, so they must **not** be used to satisfy a scope match — treat them as no match and leave +the row `>=`. Never rewrite one into a composed identity: no evidence survives about which cluster +wrote it, and inventing one manufactures provenance. + ## 6. Bootstrap — config, table, or log missing Full file formats, the literal table header, and the rationale are in diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index 04da0bc..5c10e97 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -6,7 +6,7 @@ read it. If it does not exist, the skills bootstrap it (below). ```json { "enabled": true, - "digest_cluster": "", + "digest_cluster": "@", "digest_user": "", "table": "~/.claude/slurm-sizing/table.md", "log": "~/.claude/slurm-sizing/jobs.tsv", @@ -22,14 +22,16 @@ read it. If it does not exist, the skills bootstrap it (below). } ``` -The two `<...>` values above are placeholders — fill them in with your own. Do not copy them -verbatim, and in particular do not copy a cluster name from anyone else's config: a wrong -`digest_cluster` is the one setting that corrupts every recommendation silently (below). +Every `<...>` above is a placeholder — fill them in with your own. `digest_cluster` is the composed +identity `@`, both parts read from `scontrol show config` (below); do +not copy a cluster identity from anyone else's config, and do not shorten it to the bare +`ClusterName`. A wrong `digest_cluster` is the one setting that corrupts every recommendation +silently. | Key | Default | Meaning | |---|---|---| | `enabled` | `true` | Master switch. When `false`, both skills stay silent and do nothing (see "Declining"). | -| `digest_cluster` | **none — must be set** | The Slurm cluster whose weekly usage digest feeds this system. Compared against the local cluster before anything is consulted or logged. Must be the cluster's exact Slurm name (below). | +| `digest_cluster` | **none — must be set** | The cluster whose weekly usage digest feeds this system, as the composed identity `@` (e.g. `alpha@ctl-1`). Compared against the local identity before anything is consulted or logged. A bare `ClusterName` is **not** sufficient — see "Why the controller host is part of the identity". | | `digest_user` | the invoking `$USER` | Which user's rows to merge. The digest carries a `User` column and may contain several people's jobs. | | `table` | `~/.claude/slurm-sizing/table.md` | The sizing table. | | `log` | `~/.claude/slurm-sizing/jobs.tsv` | The submission log where scope is recorded. | @@ -92,38 +94,84 @@ install has an empty table, so every job is "unknown" — which `slurm-sizing` r measure, never as licence to guess. If your site does not send such a digest, the skills have no input and there is nothing to configure; see "Declining" below. -## Determining the local cluster — one procedure, used everywhere +## Determining the local cluster identity — one procedure, used everywhere `slurm-sizing` (its cluster gate), `slurm-digest` (its `sacct` enrichment guard) and bootstrap all -need the **local** Slurm cluster's name. They all use exactly this procedure — do not substitute +need the **local cluster identity**. They all use exactly this procedure — do not substitute another: -1. `sacctmgr -n -P list cluster format=Cluster` — take the single name it prints. -2. If that prints nothing or fails: `scontrol show config | grep ClusterName` and take the value - to the right of the `=`, trimmed of whitespace. -3. If neither yields a name, the local cluster is **unknown**. Say so and stop. "Unknown" is never - treated as a match. +1. Read both fields from one call: -**Never use `hostname` for this.** It returns a node's name, not a Slurm cluster's name; the two -are different strings on essentially every site, so comparing a hostname against `digest_cluster` -fails silently and permanently disables whatever it was guarding. - -## `digest_cluster` must be the exact Slurm cluster name - -`digest_cluster` is **exact-matched** — against the local cluster name from the procedure above, -and against the `cluster` column of every log row. A colloquial or approximate answer ("the -cluster", "our HPC", a hostname, a capitalised variant) is not a failure that announces itself: it -makes `slurm-sizing`'s gate never match, so the skill goes inert forever, or it makes the digest -join never match, so every recommendation carries `>=` forever. + ```bash + scontrol show config | grep -E 'ClusterName|SlurmctldHost\[0\]' + ``` -Therefore **bootstrap does not ask for it in free text.** It runs the query above, shows the user -the exact string, and offers that string as the answer (see Bootstrap). And the `cluster` column -written into every log row must carry that same exact string — it is what the digest merge +2. Take the value to the right of each `=`, trimmed of whitespace: `ClusterName`, and the + controller host from `SlurmctldHost[0]`. +3. **Compose the identity as `@`** — e.g. `alpha@ctl-1`, + `alpha@ctl-2`. This composed string *is* the cluster identity everywhere in this system: it is + what `digest_cluster` holds, what goes in the log's `cluster` column, and what the local-vs-digest + gate compares. +4. If either field is missing, the local identity is **unknown**. Say so and stop. Do **not** fall + back to the bare `ClusterName` and treat it as the identity — that is precisely the unsafe key + this composition exists to replace. "Unknown" is never treated as a match. + +**Never use `hostname` for this.** It returns a node's name, not a cluster's identity; the two are +different strings on essentially every site, so comparing a hostname against `digest_cluster` fails +silently and permanently disables whatever it was guarding. + +### Why the controller host is part of the identity + +**A Slurm `ClusterName` is not guaranteed to be unique across the clusters one person can reach.** +It is a locally chosen label, and nothing stops two independent clusters from carrying the same one. +This is not hypothetical: **two clusters reachable from one shared home directory were found +reporting the same `ClusterName` while having entirely separate controllers, separate accounting +databases, and independent job-ID spaces.** The same job number named two different jobs, submitted +a year apart, depending on which host you asked. + +That matters here because `~/.claude` is often on shared/NFS storage, so **one submission log +receives rows from every cluster the user touches** — and under a bare-name key they are all tagged +identically. A scope recorded on one cluster then attaches to an unrelated job with the same number +on the other, which is exactly the corruption the `(cluster, jobid)` key was introduced to prevent. +A bare cluster name does not prevent it. + +Adding the controller host fixes it because two clusters with distinct job-ID spaces necessarily +have distinct controllers. **At a site where `ClusterName` is already unique, this changes nothing** +— the suffix just makes the identity explicit rather than implied. + +### Legacy log rows written before this change + +Existing logs contain **bare** cluster names. Handle them as follows, and do not silently rewrite +them: + +- A bare value that matches the `ClusterName` part of the composed identity is **unverified**: it + may have come from any cluster carrying that name, including a different one. +- **An unverified row must not be used to satisfy a scope match.** Treat it as no match — the row + stays `>=`. A scope that might belong to another cluster's job is worse than no scope. +- **Do not upgrade a bare name to a composed identity.** There is no evidence available after the + fact about which cluster wrote it; writing one in would manufacture provenance. +- Say in the report how many log rows were skipped as unverified-legacy, so the user can see the + transition happening rather than wondering why matches dropped. New rows carry the composed + identity from now on, so this fades on its own. + +## `digest_cluster` must be the exact composed identity + +`digest_cluster` holds a **`@`** string and is **exact-matched** — +against the local identity from the procedure above, and against the `cluster` column of every log +row. A colloquial or approximate answer ("the cluster", "our HPC", a hostname, a capitalised +variant, or a bare `ClusterName`) is not a failure that announces itself: it makes `slurm-sizing`'s +gate never match, so the skill goes inert forever, or it makes the digest lookup never match, so +every recommendation carries `>=` forever. + +Therefore **bootstrap does not ask for it in free text.** It runs the query above, composes the +string, shows it to the user, and offers it as the answer (see Bootstrap). And the `cluster` column +written into every log row must carry that same exact composed string — it is what the digest merge filters log rows on. **`digest_cluster` deliberately has no default.** Guessing it is the one error that silently corrupts results: sizing advice from the wrong cluster's data is worse than no advice, because job -IDs are not unique across clusters. Never infer it from the local hostname. +IDs are not unique across clusters — and, as the finding above shows, cluster *names* are not +either. Never infer it from the local hostname. ## `digest_user` and multi-user digests @@ -192,13 +240,18 @@ the user's own rows. Neither failure announces itself. If any exist, offer to point the config at them instead of creating empty files beside them. Silently starting fresh next to a populated table would strand real history. -2. **Determine `digest_cluster` by running the query, not by asking in free text.** Run the - "Determining the local cluster" procedure above, show the user the exact string it returned, and - offer it as the answer — e.g. "Slurm reports this cluster's name as ``; is that the - cluster your weekly digest covers? [use it / enter a different name / not applicable]". - - If the query returns nothing, say so and ask the user for the exact Slurm cluster name (the - value `sacctmgr` would print on that cluster), warning that it is exact-matched. - - If the user's digest covers a *different* cluster than the local one, take their name — but +2. **Derive `digest_cluster` by running the query and composing the identity — never ask for it in + free text.** Run the "Determining the local cluster identity" procedure above, **compose + `@`**, display that exact composed string, and offer it as the + answer — e.g. "This host reports `ClusterName = alpha` and `SlurmctldHost[0] = ctl-1`, so its + cluster identity is `alpha@ctl-1`; is that the cluster your weekly digest covers? [use it / + enter a different identity / not applicable]". Show both source fields, not just the result, so + the user can tell which machine they are on — the whole point of the suffix is that two hosts + can report the same `ClusterName`. + - If either field is missing, say so and ask the user for the exact composed identity of the + cluster their digest covers, warning that it is exact-matched. Do not accept or record a bare + `ClusterName` as the identity. + - If the user's digest covers a *different* cluster than the local one, take their answer — but record it verbatim, and note that `slurm-sizing` will then be inert on this machine by design. 3. **Confirm `digest_user` — show it, source it, let them correct it. Do not just write it.** The default is the invoking `$USER`, but this key exists *because* a Slurm account name and a From a5a79cf30804898fa03a11ab814e94643b3e4bfd Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Tue, 4 Aug 2026 16:51:15 -0400 Subject: [PATCH 10/15] fix(session-tools): identity suffix becomes the short hostname, add a fallback chain The cluster identity is now @ (e.g. alpha@login-1) rather than @. The controller name is invisible to the people using this; the submit hostname is what they say and what already appears in their logs. The trade-off is stated in all three files rather than hidden: a controller identifies the cluster, a hostname identifies where the command was typed, so a cluster with several submit hosts acquires several identities. What happens then is benign and visible -- the second host's identity will not equal the configured digest_cluster, so the gate stands down with a message instead of silently mis-keying rows. A site with several login nodes sets digest_cluster per host or standardises on one submit host. ClusterName is now derived by a three-step chain so an unreadable scontrol degrades the identity instead of making the skill inert: scontrol show config; failing that sacctmgr -n -P list cluster format=Cluster; failing both, the bare short hostname is used as the whole identity and the run says so in its report. Legacy bare cluster names are unchanged: unverified, never rewritten, cannot satisfy a scope match. The one place that exclusion is no longer automatic (a hostname-only fallback identity carries no @) is called out where it lived, in slurm-digest Step 5. allowed-tools: Bash(sacctmgr *) restored to both skills -- the fallback chain makes it instructed again -- and Bash(hostname *) added to both, since the suffix is now read with hostname -s. Both bodies re-audited command by command; every instructed command is covered and nothing declared is unused. --- .../skills/slurm-digest/SKILL.md | 68 +++++++--- .../skills/slurm-sizing/SKILL.md | 75 +++++++---- .../skills/slurm-sizing/reference/config.md | 124 ++++++++++++------ 3 files changed, 189 insertions(+), 78 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index 64c3783..e9f180c 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -6,7 +6,7 @@ description: >- week-ending date it covers, then run it explicitly; this skill does not trigger on its own the way slurm-sizing does. user-invocable: true -allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(sacct *), Bash(scontrol *) +allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(sacct *), Bash(sacctmgr *), Bash(scontrol *), Bash(hostname *) --- # Slurm Digest Merge @@ -132,8 +132,8 @@ throughout is whether continuing requires *assuming* something unverified. configured `log` path (default `~/.claude/slurm-sizing/jobs.tsv`) is tab-separated and begins with `#`-prefixed comment lines before its header row `cluster jobid submitted job_name scope cwd`. - **The `cluster` column holds a composed identity, `@`** (e.g. - `alpha@ctl-1`), and so does `digest_cluster`. Digests come from the configured `digest_cluster` + **The `cluster` column holds a composed identity, `@`** (e.g. + `alpha@login-1`), and so does `digest_cluster`. Digests come from the configured `digest_cluster` only; the log's rows carry their own identity, which may name a different cluster for jobs submitted elsewhere — `~/.claude` is often shared/NFS storage, so one log routinely receives rows from every cluster its owner touches. Separate clusters are separate Slurm ID namespaces — @@ -145,9 +145,19 @@ throughout is whether continuing requires *assuming* something unverified. **And matching on a bare `ClusterName` is not sufficient either.** A `ClusterName` is a locally chosen label with no uniqueness guarantee: **two clusters reachable from one shared home were found reporting the same `ClusterName` while having separate controllers, separate accounting - databases and independent job-ID spaces.** That is why the identity carries the controller host — - two clusters with independent job-ID spaces necessarily have distinct controllers. Compare the - whole composed string, never just the part before the `@`. + databases and independent job-ID spaces.** That is why the identity carries a host suffix — the + two clusters are reached from different submit hosts. Compare the whole composed string, never + just the part before the `@`. + + **The suffix is the short hostname of the submitting machine, not the controller, and that has a + cost worth knowing here.** A hostname says where the command was typed, not which cluster it + reached, so **one cluster with several submit hosts produces several identities** — + `alpha@login-1` and `alpha@login-2`. Log rows written from a second login node therefore will + **not** match `digest_cluster` and will not contribute scope. That is a visible shortfall, not a + corruption: the rows are skipped, nothing is mis-keyed, and Step 12 reports the count. If a + populated log shows many non-matching rows whose identities differ only after the `@`, say so — + the user is submitting from more than one host and should standardise, or point `digest_cluster` + at the host they actually use. **Do this with a real shell command, not by reasoning over `Read` output**, so the composite key and the tab delimiter are actually enforced. One `awk` lookup does all of it — comment @@ -180,6 +190,13 @@ throughout is whether continuing requires *assuming* something unverified. skipped as unverified-legacy, so a drop in matches is visibly the transition rather than a malfunction. New rows carry the composed identity, so this fades on its own. + **The one case where that exclusion is not automatic:** a `digest_cluster` produced by the + hostname-only fallback (Step 6's chain, step 3) has no `@`, so `$1 == c` no longer rules out bare + rows by construction — it will match a legacy row whose bare `ClusterName` happens to equal that + hostname. The rule does not change (bare names stay unverified and are never rewritten); say so + in the report when the identity is hostname-only, so a match on a bare key is visible rather than + assumed clean. + A digest row with no output line gets `scope: unknown`. **If zero rows match, say so prominently** — a silently failing lookup makes every recommendation a lower bound and the system quietly stops working. Because `slurm-sizing` refuses to log on any cluster other than @@ -207,22 +224,29 @@ throughout is whether continuing requires *assuming* something unverified. `sacct` run against this one's accounting database (`sacct -L -j ` returns nothing for it there). Determine the **local cluster identity** with the **shared procedure** in [../slurm-sizing/reference/config.md](../slurm-sizing/reference/config.md) → "Determining the - local cluster identity": + local cluster identity" — a **three-step chain** for the cluster name, then the host suffix: ```bash - scontrol show config | grep -E 'ClusterName|SlurmctldHost\[0\]' + scontrol show config | grep -E '^ClusterName' # 1. preferred + sacctmgr -n -P list cluster format=Cluster # 2. only if step 1 gave nothing + hostname -s # the suffix ``` - Take the value after each `=`, trimmed, and compose `@` — **never - `hostname`**, which returns a node name, not a cluster identity, and would therefore fail this - comparison on essentially every site while reporting the result as a deliberate skip. Compare - the **whole composed string** to `digest_cluster` from config; **comparing only the - `ClusterName` part is not sufficient** — two clusters reachable from one shared home were - found reporting the same `ClusterName` with independent job-ID spaces, so a name-only match - would query the wrong accounting database and silently attach another cluster's submit line as - this job's scope. If they differ, **skip enrichment entirely** for the whole digest — do not - query `sacct` — and say so plainly in the report (Step 12). If either field is missing, the - local identity is undeterminable: skip and say so, and do **not** fall back to the bare + Take the value after the `=` (step 1) or the single field (step 2), trimmed, and compose + `@`. **If neither step 1 nor step 2 yields a `ClusterName`**, use + the bare short hostname as the whole identity and **say so in the report (Step 12)** — the + identity is less specific than usual, and the user must be told rather than left to assume the + normal composed form. **A hostname is the suffix, never the whole identity outside that + fallback**: substituting one would fail this comparison on essentially every site while + reporting the result as a deliberate skip. Compare the **whole composed string** to + `digest_cluster` from config; **comparing only the `ClusterName` part is not sufficient** — + two clusters reachable from one shared home were found reporting the same `ClusterName` with + independent job-ID spaces, so a name-only match would query the wrong accounting database and + silently attach another cluster's submit line as this job's scope. If they differ, **skip + enrichment entirely** for the whole digest — do not query `sacct` — and say so plainly in the + report (Step 12); **a digest merged from a second submit host of the same cluster lands here**, + and skipping is the correct, visible outcome. If the hostname cannot be read, the local + identity is undeterminable: skip and say so, and do **not** fall back to the bare `ClusterName`. Never fabricate a scope to compensate. - **Batch query, one field per call — never a combined multi-field query.** Query once for every unmatched row, not per row, but query `SubmitLine` and `WorkDir` in **separate** calls: @@ -494,6 +518,14 @@ throughout is whether continuing requires *assuming* something unverified. enrichment was skipped entirely because the digest's cluster differs from the local cluster, or because the local cluster was undeterminable (the Step 6 cluster guard) — if skipped, say so explicitly rather than silently omitting the counts. + + **Also state how the local cluster identity was derived, whenever it was not the ordinary way.** + Name it if the `ClusterName` came from `sacctmgr` rather than `scontrol`, and say plainly if the + identity is a **hostname-only fallback** (neither command reported a `ClusterName`) — in that + case the key is less specific than usual, and a reader who is not told will assume the normal + `@` form. Also report log rows skipped because their identity + matched only before the `@`: those were submitted from a different host, which usually means + more than one login node is in use (Step 5). Also state: how many rows were filtered out because their `User` did not match `digest_user` (Step 3), whenever that filtering applied. diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index d2a7c96..8ff891a 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -12,7 +12,7 @@ description: >- about Slurm resource sizing. SKIP only when no cluster job is involved at any remove, or when the user has turned this system off (`"enabled": false` in its config). user-invocable: true -allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(scontrol *), Bash(grep *) +allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(scontrol *), Bash(sacctmgr *), Bash(hostname *), Bash(grep *) --- # Slurm Job Sizing @@ -40,30 +40,47 @@ data paths inside it are configurable). ## 1. Cluster gate — check this before anything else 1. Read `digest_cluster` from the config. It holds a **composed identity**, - `@` (e.g. `alpha@ctl-1`) — not a bare cluster name. + `@` (e.g. `alpha@login-1`) — not a bare cluster name. 2. Determine the **local cluster identity** using the shared procedure in - [reference/config.md](reference/config.md) → "Determining the local cluster identity": + [reference/config.md](reference/config.md) → "Determining the local cluster identity". In brief — + a **three-step chain**, so a missing `scontrol` degrades the identity instead of disabling the + skill: ```bash - scontrol show config | grep -E 'ClusterName|SlurmctldHost\[0\]' + scontrol show config | grep -E '^ClusterName' # 1. the cluster name + sacctmgr -n -P list cluster format=Cluster # 2. only if step 1 gave nothing + hostname -s # the suffix: where the command was typed ``` - Take the value after each `=`, trimmed, and compose `@`. **Never use - `hostname`** — it names a node, not a cluster. - - **Either field is missing, or the command fails:** the local identity is undeterminable. Say - so explicitly and STOP — do not consult the table, do not append to the log. **Do not fall - back to the bare `ClusterName`** — that is the unsafe key this composition replaces. - "Unknown" is never treated as a match. -3. Compare the two composed identities — an exact string match. + Take the value after the `=` (step 1) or the single field (step 2), trimmed, and compose + `@`. The hostname is the **suffix, never the whole identity** — + except in the hostname-only fallback below, where no `ClusterName` could be read at all. + - **Neither `scontrol` nor `sacctmgr` yields a `ClusterName`:** use the **bare short hostname as + the whole identity**, and **say so explicitly in this run's report** — name the fallback and + say the identity is less specific than usual. The point of reporting it is that the user is + otherwise silently working under a different key than they expect. Do not stop for this: a + reported, degraded identity beats going inert. + - **The short hostname itself cannot be read:** the identity is undeterminable. Say so + explicitly and STOP — do not consult the table, do not append to the log. **Do not fall back + to the bare `ClusterName`** — that is the unsafe key this composition replaces. "Unknown" is + never treated as a match. +3. Compare the two identities — an exact string match. - **They match:** proceed to §2. - **They differ:** say so explicitly (name both identities) and STOP. Do not consult the table and do not append to the log. This system is inert on any cluster other than the one the digest was built from — job IDs collide across clusters (see §5), so numbers from the wrong - cluster are worse than no numbers at all. - - **`digest_cluster` is a bare name with no `@`** (written before this rule existed): treat it - as **unverified** — it may name any cluster carrying that `ClusterName`. Say so and STOP, - asking the user to re-derive it through Bootstrap (§6). Do not silently upgrade it by - appending the local controller host: that asserts provenance nobody recorded. + cluster are worse than no numbers at all. **A second submit host on the same cluster lands + here** (see §5): the stand-down is the designed outcome, not a malfunction. + - **`digest_cluster` is a bare name with no `@`, and the local identity has one** (i.e. the + config predates this rule): treat it as **unverified** — it may name any cluster carrying that + `ClusterName`. Say so and STOP, asking the user to re-derive it through Bootstrap (§6). Do not + silently upgrade it by appending the local hostname: that asserts provenance nobody recorded. + - **Both sides are bare** (the local identity came from the hostname-only fallback above): an + exact match is a match — a hostname is host-specific, so this is the intended way a + fallback-bootstrapped site keeps working. Report that both sides are hostname-only. One + residual ambiguity is worth naming once: a legacy bare `ClusterName` that happens to equal + this host's short hostname would also match here, so if the config predates this rule, + re-derive it through Bootstrap (§6). ## 2. Before sizing any job — read the table first @@ -200,7 +217,7 @@ cluster jobid submitted job_name scope cwd - **Log file does not exist:** go to **Bootstrap** (§6), then append the row. - `cluster` must be the exact `digest_cluster` string from the config — the full composed - `@` identity, never a bare cluster name. The digest merge filters log + `@` identity, never a bare cluster name. The digest merge filters log rows by exact match on this column, so a variant spelling makes the row invisible, and a bare name makes it **unverifiable** (see §5). - `scope` is a short, concrete description of the workload this run did — e.g. @@ -223,9 +240,22 @@ with separate controllers, separate accounting databases and independent job-ID job number naming two unrelated jobs depending on which host you asked. Because `~/.claude` is often on shared/NFS storage, one log receives rows from both, tagged identically. That is the exact corruption this key exists to prevent, so the key must be -**`@` + jobid**. Two clusters with independent job-ID spaces -necessarily have distinct controllers, so the suffix separates them; where `ClusterName` is already -unique it changes nothing. +**`@` + jobid**. + +**What the suffix is, and what it costs — state this, don't hide it.** The suffix is the **short +hostname of the machine the command was typed on**, not the cluster's controller. Those are +different things: a controller name identifies the *cluster*, a hostname identifies *where you +were standing*. The hostname is used because it is the name the people using this system actually +say, and the name that already appears in their existing logs, while a controller name is invisible +to them. The cost is real and follows directly: **a cluster with several submit hosts acquires +several identities.** + +What happens then is benign, and this is the part to be clear about. The second host's identity +simply will not equal the configured `digest_cluster`, so the gate **stands down with a message** +(§1) rather than silently mis-keying rows into the log. The failure is visible and safe — a +stand-down, not a corruption. A site with several login nodes should either set `digest_cluster` +per host, or standardise on one submit host. Where `ClusterName` is already unique across the +clusters one person can reach, the suffix changes nothing. **Legacy rows written with a bare name are unverified.** They may have come from any cluster with that name, so they must **not** be used to satisfy a scope match — treat them as no match and leave @@ -247,8 +277,9 @@ user's own rows; neither failure announces itself. three is a directory: `test -f ~/.claude/slurm-sizing.md`, `test -f ~/.claude/slurm-jobs.tsv`, `test -d ~/.claude/slurm-digests`. If any exist, offer to point the new config at them instead of starting empty files beside real history. Then establish - `digest_cluster` — **do not ask for it in free text.** Run the §1 query, show the user the exact - string it returned, and offer it as the answer, along with the option to name a different cluster + `digest_cluster` — **do not ask for it in free text.** Run the §1 chain, compose the identity, + show the user the exact composed string **and the values it was built from**, and offer it as the + answer, along with the option to name a different cluster and the option to decline ("not applicable / I have no weekly digest"). Declining writes `{"enabled": false}`, creates nothing else, and this skill then stays silent permanently (§0). Then **confirm `digest_user` rather than just writing it**: show the value you intend to use, say diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index 5c10e97..f0193fd 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -6,7 +6,7 @@ read it. If it does not exist, the skills bootstrap it (below). ```json { "enabled": true, - "digest_cluster": "@", + "digest_cluster": "@", "digest_user": "", "table": "~/.claude/slurm-sizing/table.md", "log": "~/.claude/slurm-sizing/jobs.tsv", @@ -23,15 +23,15 @@ read it. If it does not exist, the skills bootstrap it (below). ``` Every `<...>` above is a placeholder — fill them in with your own. `digest_cluster` is the composed -identity `@`, both parts read from `scontrol show config` (below); do -not copy a cluster identity from anyone else's config, and do not shorten it to the bare -`ClusterName`. A wrong `digest_cluster` is the one setting that corrupts every recommendation -silently. +identity `@`: the cluster name from `scontrol show config` (with a +documented fallback chain, below) and the short hostname of the machine you submit from. Do not +copy a cluster identity from anyone else's config, and do not shorten it to the bare `ClusterName`. +A wrong `digest_cluster` is the one setting that corrupts every recommendation silently. | Key | Default | Meaning | |---|---|---| | `enabled` | `true` | Master switch. When `false`, both skills stay silent and do nothing (see "Declining"). | -| `digest_cluster` | **none — must be set** | The cluster whose weekly usage digest feeds this system, as the composed identity `@` (e.g. `alpha@ctl-1`). Compared against the local identity before anything is consulted or logged. A bare `ClusterName` is **not** sufficient — see "Why the controller host is part of the identity". | +| `digest_cluster` | **none — must be set** | The cluster whose weekly usage digest feeds this system, as the composed identity `@` (e.g. `alpha@login-1`). Compared against the local identity before anything is consulted or logged. A bare `ClusterName` is **not** sufficient — see "Why the identity carries a host suffix". | | `digest_user` | the invoking `$USER` | Which user's rows to merge. The digest carries a `User` column and may contain several people's jobs. | | `table` | `~/.claude/slurm-sizing/table.md` | The sizing table. | | `log` | `~/.claude/slurm-sizing/jobs.tsv` | The submission log where scope is recorded. | @@ -100,27 +100,48 @@ input and there is nothing to configure; see "Declining" below. need the **local cluster identity**. They all use exactly this procedure — do not substitute another: -1. Read both fields from one call: +1. **Get the `ClusterName`** — a three-step chain, taken in order, so that an unreadable + `scontrol` degrades the identity instead of disabling the skill: ```bash - scontrol show config | grep -E 'ClusterName|SlurmctldHost\[0\]' + scontrol show config | grep -E '^ClusterName' # 1. preferred + sacctmgr -n -P list cluster format=Cluster # 2. only if step 1 gave nothing ``` -2. Take the value to the right of each `=`, trimmed of whitespace: `ClusterName`, and the - controller host from `SlurmctldHost[0]`. -3. **Compose the identity as `@`** — e.g. `alpha@ctl-1`, - `alpha@ctl-2`. This composed string *is* the cluster identity everywhere in this system: it is + - **Step 1** — take the value to the right of the `=`, trimmed of whitespace. + - **Step 2** — if `scontrol` is unavailable, errors, or prints no `ClusterName`, ask the + accounting layer instead. Take the single field, trimmed. If it returns several lines, this + host is configured against more than one cluster and the name is ambiguous: treat that as + step 2 having failed. + - **Step 3 — neither yields a name:** use the **bare short hostname as the whole identity**, and + **say so explicitly in the run's report**. This is the documented degraded mode, and it must + be announced every time it is used: the user is otherwise silently keyed on something less + specific than usual, with no way to tell from the output. It is still better than going inert, + which is what an underivable identity used to cause. + +2. **Get the short hostname** — the machine the command was typed on, with any domain suffix + stripped: + + ```bash + hostname -s + ``` + + If `-s` is unsupported, take the first dot-separated field of `hostname`. If the hostname cannot + be read at all, the local identity is **unknown**: say so and stop. Do **not** fall back to the + bare `ClusterName` and treat it as the identity — that is precisely the unsafe key this + composition exists to replace. "Unknown" is never treated as a match. + +3. **Compose the identity as `@`** — e.g. `alpha@login-1`, + `alpha@login-2`. This composed string *is* the cluster identity everywhere in this system: it is what `digest_cluster` holds, what goes in the log's `cluster` column, and what the local-vs-digest - gate compares. -4. If either field is missing, the local identity is **unknown**. Say so and stop. Do **not** fall - back to the bare `ClusterName` and treat it as the identity — that is precisely the unsafe key - this composition exists to replace. "Unknown" is never treated as a match. + gate compares. Under the step-3 fallback it is the bare hostname alone. -**Never use `hostname` for this.** It returns a node's name, not a cluster's identity; the two are -different strings on essentially every site, so comparing a hostname against `digest_cluster` fails -silently and permanently disables whatever it was guarding. +**The hostname is the suffix, never the whole identity** — outside the step-3 fallback above. A +hostname on its own does not name a cluster; substituting one for the composed identity would fail +the comparison against `digest_cluster` on essentially every site and silently disable whatever it +was guarding. -### Why the controller host is part of the identity +### Why the identity carries a host suffix **A Slurm `ClusterName` is not guaranteed to be unique across the clusters one person can reach.** It is a locally chosen label, and nothing stops two independent clusters from carrying the same one. @@ -135,9 +156,25 @@ identically. A scope recorded on one cluster then attaches to an unrelated job w on the other, which is exactly the corruption the `(cluster, jobid)` key was introduced to prevent. A bare cluster name does not prevent it. -Adding the controller host fixes it because two clusters with distinct job-ID spaces necessarily -have distinct controllers. **At a site where `ClusterName` is already unique, this changes nothing** -— the suffix just makes the identity explicit rather than implied. +Adding a host suffix separates them, because the two clusters are reached from different submit +hosts. **At a site where `ClusterName` is already unique, this changes nothing** — the suffix just +makes the identity explicit rather than implied. + +**Why the short hostname and not the controller — and what that costs.** The controller +(`SlurmctldHost`) identifies the *cluster*; the short hostname identifies *where the command was +typed*. The hostname is used because it is the name the people using this system actually say, and +the name that already appears in their existing logs, whereas a controller name is invisible to +them. That choice has a real cost, stated here rather than buried: **a cluster with several submit +hosts acquires several identities** — `alpha@login-1` and `alpha@login-2` are two identities for +one cluster. + +**What happens then is benign, and visible.** The second host's identity will not equal the +configured `digest_cluster`, so the gate **stands down with a message** (`slurm-sizing` §1) instead +of silently mis-keying rows. Nothing is written under the wrong key; the run just declines to use +the table and says why. A site with several login nodes should either set `digest_cluster` per host, +or standardise on a single submit host for the jobs it wants sized. Note that a shared `~/.claude` +holds a single config, so "per host" is only available where the home directory is not shared — +otherwise, standardising on one submit host is the workable answer. ### Legacy log rows written before this change @@ -156,14 +193,14 @@ them: ## `digest_cluster` must be the exact composed identity -`digest_cluster` holds a **`@`** string and is **exact-matched** — +`digest_cluster` holds a **`@`** string and is **exact-matched** — against the local identity from the procedure above, and against the `cluster` column of every log -row. A colloquial or approximate answer ("the cluster", "our HPC", a hostname, a capitalised -variant, or a bare `ClusterName`) is not a failure that announces itself: it makes `slurm-sizing`'s -gate never match, so the skill goes inert forever, or it makes the digest lookup never match, so -every recommendation carries `>=` forever. +row. A colloquial or approximate answer ("the cluster", "our HPC", a bare hostname where a +`ClusterName` was available, a capitalised variant, or a bare `ClusterName`) is not a failure that +announces itself: it makes `slurm-sizing`'s gate never match, so the skill goes inert forever, or it +makes the digest lookup never match, so every recommendation carries `>=` forever. -Therefore **bootstrap does not ask for it in free text.** It runs the query above, composes the +Therefore **bootstrap does not ask for it in free text.** It runs the chain above, composes the string, shows it to the user, and offers it as the answer (see Bootstrap). And the `cluster` column written into every log row must carry that same exact composed string — it is what the digest merge filters log rows on. @@ -171,7 +208,10 @@ filters log rows on. **`digest_cluster` deliberately has no default.** Guessing it is the one error that silently corrupts results: sizing advice from the wrong cluster's data is worse than no advice, because job IDs are not unique across clusters — and, as the finding above shows, cluster *names* are not -either. Never infer it from the local hostname. +either. The local hostname supplies the identity's **suffix** and nothing more: it is never, on its +own, an inference about which cluster the user's digest covers. (The one exception is the step-3 +fallback above, where no `ClusterName` could be read at all — and that is disclosed to the user at +bootstrap and reported on every run that uses it, not inferred silently.) ## `digest_user` and multi-user digests @@ -240,17 +280,25 @@ the user's own rows. Neither failure announces itself. If any exist, offer to point the config at them instead of creating empty files beside them. Silently starting fresh next to a populated table would strand real history. -2. **Derive `digest_cluster` by running the query and composing the identity — never ask for it in +2. **Derive `digest_cluster` by running the chain and composing the identity — never ask for it in free text.** Run the "Determining the local cluster identity" procedure above, **compose - `@`**, display that exact composed string, and offer it as the - answer — e.g. "This host reports `ClusterName = alpha` and `SlurmctldHost[0] = ctl-1`, so its - cluster identity is `alpha@ctl-1`; is that the cluster your weekly digest covers? [use it / - enter a different identity / not applicable]". Show both source fields, not just the result, so + `@`**, display that exact composed string, and offer it as the + answer — e.g. "This host reports `ClusterName = alpha` and `hostname -s = login-1`, so its + cluster identity is `alpha@login-1`; is that the cluster your weekly digest covers? [use it / + enter a different identity / not applicable]". Show both source values, not just the result, so the user can tell which machine they are on — the whole point of the suffix is that two hosts can report the same `ClusterName`. - - If either field is missing, say so and ask the user for the exact composed identity of the - cluster their digest covers, warning that it is exact-matched. Do not accept or record a bare - `ClusterName` as the identity. + - **Say which step of the chain supplied the `ClusterName`** when it was not step 1, and say + plainly if the identity is a **hostname-only fallback** (step 3): "neither `scontrol` nor + `sacctmgr` reported a `ClusterName` here, so the identity is just this host's name, `login-1` + — less specific than usual". The user is deciding what to record; they need to know the value + is degraded before they approve it. + - Mention the multi-submit-host consequence when offering the value: if they submit from more + than one login node, this identity covers **this one**, and `slurm-sizing` will stand down on + the others (see "Why the identity carries a host suffix"). + - If the hostname cannot be read at all, say so and ask the user for the exact composed identity + of the cluster their digest covers, warning that it is exact-matched. Do not accept or record + a bare `ClusterName` as the identity. - If the user's digest covers a *different* cluster than the local one, take their answer — but record it verbatim, and note that `slurm-sizing` will then be inert on this machine by design. 3. **Confirm `digest_user` — show it, source it, let them correct it. Do not just write it.** From 0efa66071cf2cb0a8c0cea32e21fd41831d97f76 Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Tue, 4 Aug 2026 16:59:57 -0400 Subject: [PATCH 11/15] fix(session-tools): resolve the host suffix via SLURM_SUBMIT_HOST, drop compound probes Inside an allocation the hostname is the compute node, so the identity was wrong and the skill silently stood down for the whole session. Measured on a real cluster: on the login node `hostname -s` = login-1 and SLURM_SUBMIT_HOST is unset; inside `srun --pty` on the same cluster `hostname -s` = the compute node while SLURM_SUBMIT_HOST is still login-1. Interactive srun --pty sessions are ordinary working practice, not a corner case, so every such session minted @, matched no configured digest_cluster, and stood the skill down until the session ended -- looking broken rather than degraded. The host component is now ${SLURM_SUBMIT_HOST:-$(hostname -s)}: Slurm sets that variable exactly when hostname is the wrong answer and leaves it unset on a login node, so one expression is right in both contexts. Applied in all three files, including the fallback chain's step 3 (bare host as the whole identity), which now resolves the host the same way. The reason is documented at each site so a future reader does not "simplify" it back to a bare hostname. allowed-tools: Bash(echo *) added to both skills -- reading the variable means printing it. The bootstrap layout probe no longer uses `test -f ... && echo`; it is three bare `test` commands whose exit status is read, so it stays inside Bash(test *) instead of relying on a prefix pattern to cover a compound. Both skill bodies and config.md re-audited command by command; every instructed command is covered and nothing declared is unused. --- .../skills/slurm-digest/SKILL.md | 47 ++++++---- .../skills/slurm-sizing/SKILL.md | 59 +++++++----- .../skills/slurm-sizing/reference/config.md | 91 ++++++++++++------- 3 files changed, 123 insertions(+), 74 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index e9f180c..bc60f2f 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -6,7 +6,7 @@ description: >- week-ending date it covers, then run it explicitly; this skill does not trigger on its own the way slurm-sizing does. user-invocable: true -allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(sacct *), Bash(sacctmgr *), Bash(scontrol *), Bash(hostname *) +allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(grep *), Bash(awk *), Bash(sacct *), Bash(sacctmgr *), Bash(scontrol *), Bash(hostname *), Bash(echo *) --- # Slurm Digest Merge @@ -34,7 +34,10 @@ follow its "Bootstrap" section first rather than guessing any of these values. T in this skill, on first use — it probes for a pre-plugin layout with `test -f ~/.claude/slurm-sizing.md`, `test -f ~/.claude/slurm-jobs.tsv`, `test -d ~/.claude/slurm-digests` (the third is a directory, which `Read` cannot distinguish from a missing path), confirms `digest_cluster` and `digest_user` -with the user, and creates the archive directory with `mkdir -p`. +with the user, and creates the archive directory with `mkdir -p`. **Issue those three probes as +bare commands and read the exit status (`0` = present) — do not chain them with `&&`/`||` or an +`echo`**, since a compound command is not reliably covered by the `Bash(test *)` permission this +skill declares. **If `enabled` is `false`**, the user has declined this system. Merge nothing. Report that it is turned off and offer to re-enable it (set `"enabled": true` and run bootstrap); proceed only if @@ -132,7 +135,7 @@ throughout is whether continuing requires *assuming* something unverified. configured `log` path (default `~/.claude/slurm-sizing/jobs.tsv`) is tab-separated and begins with `#`-prefixed comment lines before its header row `cluster jobid submitted job_name scope cwd`. - **The `cluster` column holds a composed identity, `@`** (e.g. + **The `cluster` column holds a composed identity, `@`** (e.g. `alpha@login-1`), and so does `digest_cluster`. Digests come from the configured `digest_cluster` only; the log's rows carry their own identity, which may name a different cluster for jobs submitted elsewhere — `~/.claude` is often shared/NFS storage, so one log routinely receives @@ -191,10 +194,10 @@ throughout is whether continuing requires *assuming* something unverified. malfunction. New rows carry the composed identity, so this fades on its own. **The one case where that exclusion is not automatic:** a `digest_cluster` produced by the - hostname-only fallback (Step 6's chain, step 3) has no `@`, so `$1 == c` no longer rules out bare + host-only fallback (Step 6's chain, step 3) has no `@`, so `$1 == c` no longer rules out bare rows by construction — it will match a legacy row whose bare `ClusterName` happens to equal that hostname. The rule does not change (bare names stay unverified and are never rewritten); say so - in the report when the identity is hostname-only, so a match on a bare key is visible rather than + in the report when the identity is host-only, so a match on a bare key is visible rather than assumed clean. A digest row with no output line gets `scope: unknown`. **If zero rows match, say so @@ -229,25 +232,35 @@ throughout is whether continuing requires *assuming* something unverified. ```bash scontrol show config | grep -E '^ClusterName' # 1. preferred sacctmgr -n -P list cluster format=Cluster # 2. only if step 1 gave nothing - hostname -s # the suffix + echo "${SLURM_SUBMIT_HOST:-$(hostname -s)}" # the suffix ``` + **`SLURM_SUBMIT_HOST` first, `hostname -s` only as its fallback — do not "simplify" this back + to a bare `hostname`.** Inside an allocation (`srun --pty bash`, or anything under `sbatch`) + `hostname` names the **compute node**, not the machine the user submitted from; Slurm sets + `SLURM_SUBMIT_HOST` exactly in those contexts and leaves it unset on a login node, so the one + expression is correct in both. **The identity must not change just because the merge is being + run from inside an interactive session** — otherwise this guard would compare `alpha@node-17` + against `digest_cluster`, conclude the digest is from another cluster, and skip enrichment for + the whole merge while reporting it as a deliberate skip. + Take the value after the `=` (step 1) or the single field (step 2), trimmed, and compose - `@`. **If neither step 1 nor step 2 yields a `ClusterName`**, use - the bare short hostname as the whole identity and **say so in the report (Step 12)** — the - identity is less specific than usual, and the user must be told rather than left to assume the - normal composed form. **A hostname is the suffix, never the whole identity outside that - fallback**: substituting one would fail this comparison on essentially every site while - reporting the result as a deliberate skip. Compare the **whole composed string** to + `@`. **If neither step 1 nor step 2 yields a + `ClusterName`**, use the bare short submit hostname as the whole identity and **say so in the + report (Step 12)** — the identity is less specific than usual, and the user must be told rather + than left to assume the normal composed form. **The host is the suffix, never the whole + identity outside that fallback**: substituting one would fail this comparison on essentially + every site while reporting the result as a deliberate skip. Compare the **whole composed + string** to `digest_cluster` from config; **comparing only the `ClusterName` part is not sufficient** — two clusters reachable from one shared home were found reporting the same `ClusterName` with independent job-ID spaces, so a name-only match would query the wrong accounting database and silently attach another cluster's submit line as this job's scope. If they differ, **skip enrichment entirely** for the whole digest — do not query `sacct` — and say so plainly in the report (Step 12); **a digest merged from a second submit host of the same cluster lands here**, - and skipping is the correct, visible outcome. If the hostname cannot be read, the local - identity is undeterminable: skip and say so, and do **not** fall back to the bare - `ClusterName`. Never fabricate a scope to compensate. + and skipping is the correct, visible outcome. If neither `SLURM_SUBMIT_HOST` nor a hostname can + be read, the local identity is undeterminable: skip and say so, and do **not** fall back to the + bare `ClusterName`. Never fabricate a scope to compensate. - **Batch query, one field per call — never a combined multi-field query.** Query once for every unmatched row, not per row, but query `SubmitLine` and `WorkDir` in **separate** calls: `sacct -j --format=JobID,SubmitLine --parsable2 --noheader` and, only @@ -521,9 +534,9 @@ throughout is whether continuing requires *assuming* something unverified. **Also state how the local cluster identity was derived, whenever it was not the ordinary way.** Name it if the `ClusterName` came from `sacctmgr` rather than `scontrol`, and say plainly if the - identity is a **hostname-only fallback** (neither command reported a `ClusterName`) — in that + identity is a **host-only fallback** (neither command reported a `ClusterName`) — in that case the key is less specific than usual, and a reader who is not told will assume the normal - `@` form. Also report log rows skipped because their identity + `@` form. Also report log rows skipped because their identity matched only before the `@`: those were submitted from a different host, which usually means more than one login node is in use (Step 5). Also state: how many rows were filtered out because their `User` did not match diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index 8ff891a..09e0b78 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -12,7 +12,7 @@ description: >- about Slurm resource sizing. SKIP only when no cluster job is involved at any remove, or when the user has turned this system off (`"enabled": false` in its config). user-invocable: true -allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(scontrol *), Bash(sacctmgr *), Bash(hostname *), Bash(grep *) +allowed-tools: Read, Write, Edit, Bash(test *), Bash(mkdir *), Bash(scontrol *), Bash(sacctmgr *), Bash(hostname *), Bash(echo *), Bash(grep *) --- # Slurm Job Sizing @@ -40,7 +40,7 @@ data paths inside it are configurable). ## 1. Cluster gate — check this before anything else 1. Read `digest_cluster` from the config. It holds a **composed identity**, - `@` (e.g. `alpha@login-1`) — not a bare cluster name. + `@` (e.g. `alpha@login-1`) — not a bare cluster name. 2. Determine the **local cluster identity** using the shared procedure in [reference/config.md](reference/config.md) → "Determining the local cluster identity". In brief — a **three-step chain**, so a missing `scontrol` degrades the identity instead of disabling the @@ -49,19 +49,30 @@ data paths inside it are configurable). ```bash scontrol show config | grep -E '^ClusterName' # 1. the cluster name sacctmgr -n -P list cluster format=Cluster # 2. only if step 1 gave nothing - hostname -s # the suffix: where the command was typed + echo "${SLURM_SUBMIT_HOST:-$(hostname -s)}" # the suffix: where the command was typed ``` Take the value after the `=` (step 1) or the single field (step 2), trimmed, and compose - `@`. The hostname is the **suffix, never the whole identity** — - except in the hostname-only fallback below, where no `ClusterName` could be read at all. - - **Neither `scontrol` nor `sacctmgr` yields a `ClusterName`:** use the **bare short hostname as - the whole identity**, and **say so explicitly in this run's report** — name the fallback and - say the identity is less specific than usual. The point of reporting it is that the user is - otherwise silently working under a different key than they expect. Do not stop for this: a - reported, degraded identity beats going inert. - - **The short hostname itself cannot be read:** the identity is undeterminable. Say so - explicitly and STOP — do not consult the table, do not append to the log. **Do not fall back + `@`. The host is the **suffix, never the whole identity** — + except in the host-only fallback below, where no `ClusterName` could be read at all. + + **`SLURM_SUBMIT_HOST` first, `hostname -s` only as its fallback — do not "simplify" this back to + a bare `hostname`.** Inside an allocation (an `srun --pty bash` session, or anything running + under `sbatch`) `hostname` names the **compute node**, not the machine the user submitted from. + Slurm sets `SLURM_SUBMIT_HOST` exactly in those contexts and leaves it unset on a login node, so + the one expression is correct in both. **The identity must not change just because the user is + working in an interactive session**: a bare `hostname` would mint `alpha@node-17` for that whole + session, equal no configured `digest_cluster`, and stand the skill down until the session ended — + which looks broken rather than degraded. Interactive `srun --pty` sessions are ordinary working + practice, not a corner case. + - **Neither `scontrol` nor `sacctmgr` yields a `ClusterName`:** use the **bare short submit + hostname as the whole identity** — resolved the same way, `SLURM_SUBMIT_HOST` before + `hostname -s` — and **say so explicitly in this run's report**: name the fallback and say the + identity is less specific than usual. The point of reporting it is that the user is otherwise + silently working under a different key than they expect. Do not stop for this: a reported, + degraded identity beats going inert. + - **Neither `SLURM_SUBMIT_HOST` nor a hostname can be read:** the identity is undeterminable. Say + so explicitly and STOP — do not consult the table, do not append to the log. **Do not fall back to the bare `ClusterName`** — that is the unsafe key this composition replaces. "Unknown" is never treated as a match. 3. Compare the two identities — an exact string match. @@ -74,13 +85,13 @@ data paths inside it are configurable). - **`digest_cluster` is a bare name with no `@`, and the local identity has one** (i.e. the config predates this rule): treat it as **unverified** — it may name any cluster carrying that `ClusterName`. Say so and STOP, asking the user to re-derive it through Bootstrap (§6). Do not - silently upgrade it by appending the local hostname: that asserts provenance nobody recorded. - - **Both sides are bare** (the local identity came from the hostname-only fallback above): an - exact match is a match — a hostname is host-specific, so this is the intended way a - fallback-bootstrapped site keeps working. Report that both sides are hostname-only. One - residual ambiguity is worth naming once: a legacy bare `ClusterName` that happens to equal - this host's short hostname would also match here, so if the config predates this rule, - re-derive it through Bootstrap (§6). + silently upgrade it by appending the local host: that asserts provenance nobody recorded. + - **Both sides are bare** (the local identity came from the host-only fallback above): an exact + match is a match — a submit hostname is host-specific, so this is the intended way a + fallback-bootstrapped site keeps working. Report that both sides are host-only. One residual + ambiguity is worth naming once: a legacy bare `ClusterName` that happens to equal this host's + short submit hostname would also match here, so if the config predates this rule, re-derive it + through Bootstrap (§6). ## 2. Before sizing any job — read the table first @@ -217,7 +228,7 @@ cluster jobid submitted job_name scope cwd - **Log file does not exist:** go to **Bootstrap** (§6), then append the row. - `cluster` must be the exact `digest_cluster` string from the config — the full composed - `@` identity, never a bare cluster name. The digest merge filters log + `@` identity, never a bare cluster name. The digest merge filters log rows by exact match on this column, so a variant spelling makes the row invisible, and a bare name makes it **unverifiable** (see §5). - `scope` is a short, concrete description of the workload this run did — e.g. @@ -240,7 +251,7 @@ with separate controllers, separate accounting databases and independent job-ID job number naming two unrelated jobs depending on which host you asked. Because `~/.claude` is often on shared/NFS storage, one log receives rows from both, tagged identically. That is the exact corruption this key exists to prevent, so the key must be -**`@` + jobid**. +**`@` + jobid**. **What the suffix is, and what it costs — state this, don't hide it.** The suffix is the **short hostname of the machine the command was typed on**, not the cluster's controller. Those are @@ -275,8 +286,10 @@ user's own rows; neither failure announces itself. - **Config missing:** check first for a pre-plugin layout — probe with `test`, since one of the three is a directory: `test -f ~/.claude/slurm-sizing.md`, `test -f ~/.claude/slurm-jobs.tsv`, - `test -d ~/.claude/slurm-digests`. If any exist, offer to point the new config at them instead of - starting empty files beside real history. Then establish + `test -d ~/.claude/slurm-digests`. **Issue these as three bare commands and read the exit status + (`0` = present) — do not chain them with `&&`/`||` or an `echo`**, since a compound command is not + reliably covered by the `Bash(test *)` permission this skill declares. If any exist, offer to + point the new config at them instead of starting empty files beside real history. Then establish `digest_cluster` — **do not ask for it in free text.** Run the §1 chain, compose the identity, show the user the exact composed string **and the values it was built from**, and offer it as the answer, along with the option to name a different cluster diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index f0193fd..9827d75 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -6,7 +6,7 @@ read it. If it does not exist, the skills bootstrap it (below). ```json { "enabled": true, - "digest_cluster": "@", + "digest_cluster": "@", "digest_user": "", "table": "~/.claude/slurm-sizing/table.md", "log": "~/.claude/slurm-sizing/jobs.tsv", @@ -23,15 +23,16 @@ read it. If it does not exist, the skills bootstrap it (below). ``` Every `<...>` above is a placeholder — fill them in with your own. `digest_cluster` is the composed -identity `@`: the cluster name from `scontrol show config` (with a -documented fallback chain, below) and the short hostname of the machine you submit from. Do not -copy a cluster identity from anyone else's config, and do not shorten it to the bare `ClusterName`. +identity `@`: the cluster name from `scontrol show config` +(with a documented fallback chain, below) and the short hostname of the machine you **submit** from +— which is not always the machine you are typing on, see below. Do not copy a cluster identity from +anyone else's config, and do not shorten it to the bare `ClusterName`. A wrong `digest_cluster` is the one setting that corrupts every recommendation silently. | Key | Default | Meaning | |---|---|---| | `enabled` | `true` | Master switch. When `false`, both skills stay silent and do nothing (see "Declining"). | -| `digest_cluster` | **none — must be set** | The cluster whose weekly usage digest feeds this system, as the composed identity `@` (e.g. `alpha@login-1`). Compared against the local identity before anything is consulted or logged. A bare `ClusterName` is **not** sufficient — see "Why the identity carries a host suffix". | +| `digest_cluster` | **none — must be set** | The cluster whose weekly usage digest feeds this system, as the composed identity `@` (e.g. `alpha@login-1`). Compared against the local identity before anything is consulted or logged. A bare `ClusterName` is **not** sufficient — see "Why the identity carries a host suffix". | | `digest_user` | the invoking `$USER` | Which user's rows to merge. The digest carries a `User` column and may contain several people's jobs. | | `table` | `~/.claude/slurm-sizing/table.md` | The sizing table. | | `log` | `~/.claude/slurm-sizing/jobs.tsv` | The submission log where scope is recorded. | @@ -113,33 +114,47 @@ another: accounting layer instead. Take the single field, trimmed. If it returns several lines, this host is configured against more than one cluster and the name is ambiguous: treat that as step 2 having failed. - - **Step 3 — neither yields a name:** use the **bare short hostname as the whole identity**, and - **say so explicitly in the run's report**. This is the documented degraded mode, and it must - be announced every time it is used: the user is otherwise silently keyed on something less - specific than usual, with no way to tell from the output. It is still better than going inert, - which is what an underivable identity used to cause. - -2. **Get the short hostname** — the machine the command was typed on, with any domain suffix + - **Step 3 — neither yields a name:** use the **bare submit host as the whole identity** — + resolved exactly as in step 2 below, not by a bare `hostname` — and **say so explicitly in the + run's report**. This is the documented degraded mode, and it must be announced every time it + is used: the user is otherwise silently keyed on something less specific than usual, with no + way to tell from the output. It is still better than going inert, which is what an underivable + identity used to cause. + +2. **Get the short submit hostname** — the machine the command was typed on, with any domain suffix stripped: ```bash - hostname -s + echo "${SLURM_SUBMIT_HOST:-$(hostname -s)}" ``` - If `-s` is unsupported, take the first dot-separated field of `hostname`. If the hostname cannot - be read at all, the local identity is **unknown**: say so and stop. Do **not** fall back to the - bare `ClusterName` and treat it as the identity — that is precisely the unsafe key this - composition exists to replace. "Unknown" is never treated as a match. - -3. **Compose the identity as `@`** — e.g. `alpha@login-1`, + **Prefer `SLURM_SUBMIT_HOST`; fall back to `hostname -s`. Do not "simplify" this back to a bare + `hostname`.** Inside an allocation — an `srun --pty bash` session, or anything running under + `sbatch` — `hostname` names the **compute node**, not the machine the user submitted from. Slurm + sets `SLURM_SUBMIT_HOST` exactly in those contexts, and leaves it unset on a login node, so this + one expression resolves to the submit host in both. Measured on a real cluster: on the login node + `hostname -s` gives `login-1` and `SLURM_SUBMIT_HOST` is unset; inside `srun --pty` on the same + cluster `hostname -s` gives `node-17` while `SLURM_SUBMIT_HOST` is still `login-1`. **The identity + must not change just because the user is working in an interactive session** — a bare `hostname` + would mint `alpha@node-17` for the whole session, which matches no configured `digest_cluster`, + so the skill would stand down for that entire session and look broken. This is not a corner case: + interactive `srun --pty` sessions are ordinary working practice. + + If `hostname -s` is unsupported, take the first dot-separated field of `hostname`. If neither + `SLURM_SUBMIT_HOST` nor a hostname can be read at all, the local identity is **unknown**: say so + and stop. Do **not** fall back to the bare `ClusterName` and treat it as the identity — that is + precisely the unsafe key this composition exists to replace. "Unknown" is never treated as a + match. + +3. **Compose the identity as `@`** — e.g. `alpha@login-1`, `alpha@login-2`. This composed string *is* the cluster identity everywhere in this system: it is what `digest_cluster` holds, what goes in the log's `cluster` column, and what the local-vs-digest - gate compares. Under the step-3 fallback it is the bare hostname alone. + gate compares. Under the step-3 fallback it is the bare submit hostname alone. -**The hostname is the suffix, never the whole identity** — outside the step-3 fallback above. A -hostname on its own does not name a cluster; substituting one for the composed identity would fail -the comparison against `digest_cluster` on essentially every site and silently disable whatever it -was guarding. +**The host is the suffix, never the whole identity** — outside the step-3 fallback above. A hostname +on its own does not name a cluster; substituting one for the composed identity would fail the +comparison against `digest_cluster` on essentially every site and silently disable whatever it was +guarding. ### Why the identity carries a host suffix @@ -193,7 +208,7 @@ them: ## `digest_cluster` must be the exact composed identity -`digest_cluster` holds a **`@`** string and is **exact-matched** — +`digest_cluster` holds a **`@`** string and is **exact-matched** — against the local identity from the procedure above, and against the `cluster` column of every log row. A colloquial or approximate answer ("the cluster", "our HPC", a bare hostname where a `ClusterName` was available, a capitalised variant, or a bare `ClusterName`) is not a failure that @@ -273,26 +288,34 @@ the user's own rows. Neither failure announces itself. cleanly as for a file (a `Read` cannot: it fails on a missing path and on a directory alike): ```bash - test -f ~/.claude/slurm-sizing.md && echo "found: legacy table" - test -f ~/.claude/slurm-jobs.tsv && echo "found: legacy log" - test -d ~/.claude/slurm-digests && echo "found: legacy archive dir" + test -f ~/.claude/slurm-sizing.md + test -f ~/.claude/slurm-jobs.tsv + test -d ~/.claude/slurm-digests ``` + **Three bare commands, one per path — read the exit status (`0` = present).** Do not join them + with `&&`/`||` or wrap them in an `echo`: a compound command is not reliably covered by the + `Bash(test *)` permission these skills declare, and a probe that needs an extra permission to run + is a probe that silently doesn't run. + If any exist, offer to point the config at them instead of creating empty files beside them. Silently starting fresh next to a populated table would strand real history. 2. **Derive `digest_cluster` by running the chain and composing the identity — never ask for it in free text.** Run the "Determining the local cluster identity" procedure above, **compose - `@`**, display that exact composed string, and offer it as the - answer — e.g. "This host reports `ClusterName = alpha` and `hostname -s = login-1`, so its + `@`**, display that exact composed string, and offer it as + the answer — e.g. "This host reports `ClusterName = alpha` and submit host `login-1`, so its cluster identity is `alpha@login-1`; is that the cluster your weekly digest covers? [use it / enter a different identity / not applicable]". Show both source values, not just the result, so the user can tell which machine they are on — the whole point of the suffix is that two hosts can report the same `ClusterName`. - **Say which step of the chain supplied the `ClusterName`** when it was not step 1, and say - plainly if the identity is a **hostname-only fallback** (step 3): "neither `scontrol` nor - `sacctmgr` reported a `ClusterName` here, so the identity is just this host's name, `login-1` - — less specific than usual". The user is deciding what to record; they need to know the value - is degraded before they approve it. + plainly if the identity is a **host-only fallback** (step 3): "neither `scontrol` nor + `sacctmgr` reported a `ClusterName` here, so the identity is just this submit host's name, + `login-1` — less specific than usual". The user is deciding what to record; they need to know + the value is degraded before they approve it. + - **If the value came from `SLURM_SUBMIT_HOST` rather than `hostname -s`, say so** — the user is + inside an allocation, and seeing the login node's name offered while `hostname` reports a + compute node would otherwise look like a mistake. - Mention the multi-submit-host consequence when offering the value: if they submit from more than one login node, this identity covers **this one**, and `slurm-sizing` will stand down on the others (see "Why the identity carries a host suffix"). From f56d76347907a3378ab99acf5b04bfe020ffff5a Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Tue, 4 Aug 2026 17:04:09 -0400 Subject: [PATCH 12/15] fix(session-tools): strip a domain suffix from whichever source supplied the host SLURM_SUBMIT_HOST and `hostname -s` can disagree in form: some sites record an FQDN in the variable while `hostname -s` returns the short name. Normalising only one of them would make the login-node identity and the in-allocation identity differ by a domain suffix, standing the gate down inside every allocation -- the exact failure SLURM_SUBMIT_HOST was introduced to remove, on sites we cannot test. The rule is now: strip any domain suffix from whichever source supplied the value, taking the first dot-separated field, so SLURM_SUBMIT_HOST= login-1.example.edu and `hostname -s` = login-1 both yield login-1. Applied at every derivation site in all three files, including each fallback-chain step 3. No command changed -- this is a transformation of a value already being read -- so both allowed-tools lists are untouched. --- plugins/session-tools/skills/slurm-digest/SKILL.md | 9 ++++++++- plugins/session-tools/skills/slurm-sizing/SKILL.md | 9 ++++++++- .../skills/slurm-sizing/reference/config.md | 9 ++++++++- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index bc60f2f..6e8b857 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -244,9 +244,16 @@ throughout is whether continuing requires *assuming* something unverified. against `digest_cluster`, conclude the digest is from another cluster, and skip enrichment for the whole merge while reporting it as a deliberate skip. + **Strip any domain suffix from whichever source supplied the value — take the first + dot-separated field**, so `SLURM_SUBMIT_HOST=login-1.example.edu` and `hostname -s` = `login-1` + both yield `login-1`. Some sites record an FQDN in the variable and a short name from + `hostname`; normalising only one of them makes the login-node and in-allocation identities + differ by a domain suffix, which skips enrichment inside every allocation. + Take the value after the `=` (step 1) or the single field (step 2), trimmed, and compose `@`. **If neither step 1 nor step 2 yields a - `ClusterName`**, use the bare short submit hostname as the whole identity and **say so in the + `ClusterName`**, use the bare short submit hostname as the whole identity — domain suffix + stripped the same way — and **say so in the report (Step 12)** — the identity is less specific than usual, and the user must be told rather than left to assume the normal composed form. **The host is the suffix, never the whole identity outside that fallback**: substituting one would fail this comparison on essentially diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index 09e0b78..296e5ac 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -65,9 +65,16 @@ data paths inside it are configurable). session, equal no configured `digest_cluster`, and stand the skill down until the session ended — which looks broken rather than degraded. Interactive `srun --pty` sessions are ordinary working practice, not a corner case. + + **Strip any domain suffix from whichever source supplied the value — take the first + dot-separated field**, so `SLURM_SUBMIT_HOST=login-1.example.edu` and `hostname -s` = `login-1` + both yield `login-1`. Some sites record an FQDN in the variable and a short name from `hostname`; + normalising only one of them would make the login-node and in-allocation identities differ by a + domain suffix, reintroducing the same stand-down inside every allocation. - **Neither `scontrol` nor `sacctmgr` yields a `ClusterName`:** use the **bare short submit hostname as the whole identity** — resolved the same way, `SLURM_SUBMIT_HOST` before - `hostname -s` — and **say so explicitly in this run's report**: name the fallback and say the + `hostname -s`, domain suffix stripped — and **say so explicitly in this run's report**: name + the fallback and say the identity is less specific than usual. The point of reporting it is that the user is otherwise silently working under a different key than they expect. Do not stop for this: a reported, degraded identity beats going inert. diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index 9827d75..363d695 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -140,7 +140,14 @@ another: so the skill would stand down for that entire session and look broken. This is not a corner case: interactive `srun --pty` sessions are ordinary working practice. - If `hostname -s` is unsupported, take the first dot-separated field of `hostname`. If neither + **Strip any domain suffix from whichever source supplied the value — take the first + dot-separated field.** `SLURM_SUBMIT_HOST=login-1.example.edu` and `hostname -s` = `login-1` must + both yield `login-1`. Some sites record an FQDN in `SLURM_SUBMIT_HOST` while `hostname -s` + returns the short name; without this, the login-node identity and the in-allocation identity + would differ by a domain suffix and the gate would stand down inside every allocation — the exact + failure `SLURM_SUBMIT_HOST` was introduced to remove. Normalise both sources the same way, always. + + If `hostname -s` is unsupported, apply the same rule to plain `hostname`. If neither `SLURM_SUBMIT_HOST` nor a hostname can be read at all, the local identity is **unknown**: say so and stop. Do **not** fall back to the bare `ClusterName` and treat it as the identity — that is precisely the unsafe key this composition exists to replace. "Unknown" is never treated as a From 313ae7d1fce8f2356f3bd5d89f06e2a74a20b777 Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Wed, 5 Aug 2026 11:00:10 -0400 Subject: [PATCH 13/15] fix(session-tools): split the command substitution, correct the bare-probe rationale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The layout probes were mandated as three bare `test` commands on the grounds that "a compound command is not reliably covered by the Bash(test *) permission this skill declares" — while the same documents prescribed `scontrol show config | grep -E '^ClusterName'` and `echo "${SLURM_SUBMIT_HOST:-$(hostname -s)}"`. The stated objection applied to those two just as much, so the rule contradicted itself and one of the two had to be wrong. The pipe was the wrong target. A pipe whose halves are separately declared is covered — `Bash(scontrol *)` together with `Bash(grep *)` — so it stays. What cannot be checked before it runs is command substitution, so the submit-host probe becomes two bare commands: `echo "$SLURM_SUBMIT_HOST"`, then `hostname -s` only if that printed nothing. Same resolution order, no `$(...)`. The bare-`test` rule is kept, but on its real justification rather than the permission story: `&&` short-circuits, so the later probes never run, and a chain collapses three independent answers into one exit status that cannot say which path is missing. Co-Authored-By: Claude Opus 5 (1M context) --- .../session-tools/skills/slurm-digest/SKILL.md | 9 ++++++--- .../session-tools/skills/slurm-sizing/SKILL.md | 8 +++++--- .../skills/slurm-sizing/reference/config.md | 16 ++++++++++++---- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index 6e8b857..ae43e8a 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -35,8 +35,10 @@ in this skill, on first use — it probes for a pre-plugin layout with `test -f `test -f ~/.claude/slurm-jobs.tsv`, `test -d ~/.claude/slurm-digests` (the third is a directory, which `Read` cannot distinguish from a missing path), confirms `digest_cluster` and `digest_user` with the user, and creates the archive directory with `mkdir -p`. **Issue those three probes as -bare commands and read the exit status (`0` = present) — do not chain them with `&&`/`||` or an -`echo`**, since a compound command is not reliably covered by the `Bash(test *)` permission this +bare commands and read the exit status (`0` = present) — do not chain them with `&&`/`||`**: a +chain collapses three answers into one exit status and short-circuits, so a failure never says which +path is missing. (A pipe of two separately-declared commands is fine; it is `$(...)` command +substitution that cannot be checked before it runs.) This is the `Bash(test *)` permission this skill declares. **If `enabled` is `false`**, the user has declined this system. Merge nothing. Report that it is @@ -232,7 +234,8 @@ throughout is whether continuing requires *assuming* something unverified. ```bash scontrol show config | grep -E '^ClusterName' # 1. preferred sacctmgr -n -P list cluster format=Cluster # 2. only if step 1 gave nothing - echo "${SLURM_SUBMIT_HOST:-$(hostname -s)}" # the suffix + echo "$SLURM_SUBMIT_HOST" # 3a. the suffix, when Slurm set it + hostname -s # 3b. only if 3a printed nothing ``` **`SLURM_SUBMIT_HOST` first, `hostname -s` only as its fallback — do not "simplify" this back diff --git a/plugins/session-tools/skills/slurm-sizing/SKILL.md b/plugins/session-tools/skills/slurm-sizing/SKILL.md index 296e5ac..43620d8 100644 --- a/plugins/session-tools/skills/slurm-sizing/SKILL.md +++ b/plugins/session-tools/skills/slurm-sizing/SKILL.md @@ -49,7 +49,8 @@ data paths inside it are configurable). ```bash scontrol show config | grep -E '^ClusterName' # 1. the cluster name sacctmgr -n -P list cluster format=Cluster # 2. only if step 1 gave nothing - echo "${SLURM_SUBMIT_HOST:-$(hostname -s)}" # the suffix: where the command was typed + echo "$SLURM_SUBMIT_HOST" # 3a. the suffix, when Slurm set it + hostname -s # 3b. only if 3a printed nothing ``` Take the value after the `=` (step 1) or the single field (step 2), trimmed, and compose @@ -294,8 +295,9 @@ user's own rows; neither failure announces itself. - **Config missing:** check first for a pre-plugin layout — probe with `test`, since one of the three is a directory: `test -f ~/.claude/slurm-sizing.md`, `test -f ~/.claude/slurm-jobs.tsv`, `test -d ~/.claude/slurm-digests`. **Issue these as three bare commands and read the exit status - (`0` = present) — do not chain them with `&&`/`||` or an `echo`**, since a compound command is not - reliably covered by the `Bash(test *)` permission this skill declares. If any exist, offer to + (`0` = present) — do not chain them with `&&`/`||`**: a chain collapses three independent answers + into a single exit status, so a non-zero result never tells you *which* path is missing, and `&&` + short-circuits so the later probes never run at all. If any exist, offer to point the new config at them instead of starting empty files beside real history. Then establish `digest_cluster` — **do not ask for it in free text.** Run the §1 chain, compose the identity, show the user the exact composed string **and the values it was built from**, and offer it as the diff --git a/plugins/session-tools/skills/slurm-sizing/reference/config.md b/plugins/session-tools/skills/slurm-sizing/reference/config.md index 363d695..243f92f 100644 --- a/plugins/session-tools/skills/slurm-sizing/reference/config.md +++ b/plugins/session-tools/skills/slurm-sizing/reference/config.md @@ -125,7 +125,8 @@ another: stripped: ```bash - echo "${SLURM_SUBMIT_HOST:-$(hostname -s)}" + echo "$SLURM_SUBMIT_HOST" # prefer this + hostname -s # only if the above printed nothing ``` **Prefer `SLURM_SUBMIT_HOST`; fall back to `hostname -s`. Do not "simplify" this back to a bare @@ -301,9 +302,16 @@ the user's own rows. Neither failure announces itself. ``` **Three bare commands, one per path — read the exit status (`0` = present).** Do not join them - with `&&`/`||` or wrap them in an `echo`: a compound command is not reliably covered by the - `Bash(test *)` permission these skills declare, and a probe that needs an extra permission to run - is a probe that silently doesn't run. + with `&&`/`||`: a chain collapses three independent answers into a single exit status, so a + non-zero result never tells you *which* of the three paths is missing — and `&&` short-circuits, + so the later probes never run at all. Issue them separately and read each result. + + **On compound commands generally — the rule is about command substitution, not pipes.** A simple + pipe whose both halves are declared is fine: `scontrol show config | grep -E '^ClusterName'` is + covered by `Bash(scontrol *)` together with `Bash(grep *)`. What these skills must avoid is + **command substitution** — `$(...)` cannot be checked before it runs, so a probe written that way + may be refused outright. That is why the submit-host probe above is two bare commands rather than + the more compact `echo "${SLURM_SUBMIT_HOST:-$(hostname -s)}"`. If any exist, offer to point the config at them instead of creating empty files beside them. Silently starting fresh next to a populated table would strand real history. From 90ee50edecfd8801651430bf20c3c0f7e004af23 Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Wed, 5 Aug 2026 11:06:09 -0400 Subject: [PATCH 14/15] chore(session-tools): release 1.1.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `claude plugin update` decides whether to refresh an install by comparing the version string, not the content. The probe fix in 313ae7d therefore reached no installed copy: update reported "already at the latest version (1.1.0)" and left the recorded commit sha pointing at the pre-fix tree. Bump so the fix actually propagates. Any content-only change to a skill needs one of these; without it the marketplace source moves ahead while every install silently keeps serving the old text — the same staleness that left slurm-sizing and slurm-digest missing from an install pinned at 1.0.0. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/session-tools/.claude-plugin/plugin.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/session-tools/.claude-plugin/plugin.json b/plugins/session-tools/.claude-plugin/plugin.json index 03b3ea0..1e5b9a8 100644 --- a/plugins/session-tools/.claude-plugin/plugin.json +++ b/plugins/session-tools/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "session-tools", "description": "Session productivity tools for Claude Code. Includes self-assess for auditing sessions for errors, inefficiencies, and repeated workarounds, plus slurm-sizing/slurm-digest for sizing Slurm jobs from measured usage instead of habit.", - "version": "1.1.0", + "version": "1.1.1", "author": { "name": "Whitehead Institute" } From 82bbe63c97fd36257eb734e7c7b3baa4203ece34 Mon Sep 17 00:00:00 2001 From: Ana Karla Cepeda Diaz Date: Mon, 10 Aug 2026 09:58:35 -0400 Subject: [PATCH 15/15] fix(session-tools): derive the digest date from job IDs, fix sacct parsing traps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 1's missing-date hard stop asked the user because defaulting to today breaks idempotency: the same digest pasted on two different days would archive under two filenames, pass the duplicate check both times, and double the n column. Deriving the date from the digest's own job IDs via sacct preserves that exact property while removing the question — today's date is a property of when you pasted (changes every time), while a derived date is a property of what you pasted (stable across any future paste of the same digest). It's also more reliable than a human retyping a date out of an email. The hard stop is retained for the only case that still needs it: sacct absent/erroring, no job ID resolving, or every row carrying an Unknown end. Step 1 also now checks row overlap against the most recent archived digest, since a filename check alone can't see a digest re-sent under a different date with overlapping rows. Step 6 documents two parsing traps found in a real run: sacct's SubmitLine can embed real newlines (an srun ... python -c multi-line script splits one record across several output lines, truncating a line-by-line parse — observed live on a *_mn_verify row), so parsing must be continuation-aware, keyed on a new-record regex anchored to JobID rather than line boundaries. Second, filtering step rows with a bare grep -v '\.' is wrong because submit lines are full of dots (paths, versions) — it's the JobID field that must be tested for a dot, not the whole line. Co-Authored-By: Claude Opus 5 (1M context) --- .../skills/slurm-digest/SKILL.md | 68 ++++++++++++++++--- 1 file changed, 58 insertions(+), 10 deletions(-) diff --git a/plugins/session-tools/skills/slurm-digest/SKILL.md b/plugins/session-tools/skills/slurm-digest/SKILL.md index ae43e8a..e22f64f 100644 --- a/plugins/session-tools/skills/slurm-digest/SKILL.md +++ b/plugins/session-tools/skills/slurm-digest/SKILL.md @@ -60,9 +60,14 @@ run, rather than choosing a value. Proceeding silently past one of these is how enters the table; everything else here is built so the *conservative* outcome is the automatic one, and a hard stop marks the places where there is no conservative outcome to fall back to. -The hard stops in this skill are: **a missing week-ending date, and an already-archived digest -(Step 1)**; **the first multi-user digest encounter (Step 3)**; and **every question in Bootstrap** -(`digest_cluster`, `digest_user`, decline — see the config contract). +The hard stops in this skill are: **a week-ending date that can be neither supplied nor derived, an +already-archived digest, and a digest whose rows overlap a previously merged one (Step 1)**; **the +first multi-user digest encounter (Step 3)**; and **every question in Bootstrap** (`digest_cluster`, +`digest_user`, decline — see the config contract). + +**A missing date is no longer a hard stop by itself** — Step 1 derives it from the digest's own job +IDs via `sacct`, which is idempotent in the way that asking was protecting. The hard stop remains +for the case where derivation cannot run at all. **Step 10's scope prompt is deliberately NOT a hard stop.** Its unanswered outcome is a documented conservative one — `scope: unknown` with a `>=` prefix — and the merge completes. The distinction @@ -71,12 +76,43 @@ throughout is whether continuing requires *assuming* something unverified. ## Procedure 1. **Establish the week-ending date, then refuse duplicates (precondition — check this FIRST, - before parsing or merging anything).** The digest's date is **a required argument**, supplied - alongside the pasted table (e.g. `/session-tools:slurm-digest 2026-08-01 `). The - digest body itself contains no date column, so it cannot be recovered from the paste. - **If the date is missing: HARD STOP — ask for it, and never assume today's date.** Defaulting to - today lets the same digest pasted on two different days archive under two filenames, pass this - duplicate check both times, and double `n` — exactly the corruption this step exists to prevent. + before parsing or merging anything).** The date may be supplied as an argument + (`/session-tools:slurm-digest 2026-08-01 `). **When it is not, DERIVE it from the + digest's own job IDs — do not ask.** The digest body has no date column, but its jobs are in + Slurm's accounting database and their dates are a fact about the paste: + + ```bash + sacct -j --format=JobID,Submit,End \ + --parsable2 --noheader + ``` + + Discard `.`-suffixed step rows, take the **latest `End` date** across the remainder, and use that + as the week-ending date. Report the full span (earliest `Submit` -> latest `End`) and say plainly + that the date was derived, so the user can correct it against their digest email. + + **Why deriving is safe where defaulting to today is not** — the distinction is the whole point. + The failure this precondition prevents is the *same digest* archiving under *two filenames* on + two different days, passing the duplicate check both times and doubling `n`. Today's date is a + property of **when you pasted**, so it changes between pastes. A date derived from the job IDs is + a property of **what you pasted**, so the same digest yields the same date on any future day. + Derivation is idempotent in exactly the way the hard stop was protecting, which is why it + replaces the question rather than skipping it — and it is more reliable than a human retyping a + date out of an email. + + **Fall back to asking — HARD STOP — only when derivation cannot run:** `sacct` unavailable or + erroring, **no** job ID resolving (retention gap, or a digest from another cluster — apply Step + 6's cluster guard before trusting any of this), or every resolved row carrying an `Unknown` end. + Never assume today's date in that case; ask, exactly as before. If only *some* IDs resolve that + is enough — say how many did, since a partial resolution still pins the latest end it saw. + + Two things the derived date is **not**. It is not necessarily the calendar week boundary the + digest email names — jobs rarely end exactly on it — so a user who has the email should be + invited to correct it. And it says nothing about whether this digest overlaps a previous one: + **check row overlap as well as the filename.** Compare the digest's job IDs against those in the + most recently archived `.tsv`. The filename check cannot see a digest re-sent under a different + date carrying overlapping rows, and re-merging those inflates `n` just as badly. Report the + overlap count; a non-zero one is a HARD STOP. + With the date in hand, check whether `/YYYY-MM-DD.tsv` already exists for it (`Read` it; a "no such file" error is the not-archived case), where `` is the configured `archive` path (default `~/.claude/slurm-sizing/digests`). @@ -282,9 +318,21 @@ throughout is whether continuing requires *assuming* something unverified. live: a combined `JobID,SubmitLine,WorkDir` query interleaves `WorkDir` into the wrong position whenever a wrapped command uses one or more pipes. No replacement delimiter is safe either, since any character you pick could itself appear in a submit line. Querying one field - at a time makes each row exactly `JobID|` — a `JobID` can never contain `|`, so + at a time makes each *record* exactly `JobID|` — a `JobID` can never contain `|`, so splitting on the **first** `|` only is exact regardless of what the field holds. Keep this as two calls; do not re-merge them into one query. + - **A record is not always a LINE — a submit line can contain newlines.** `srun ... python -c` + with a multi-line script embeds real newlines in `SubmitLine`, so one record spans several + output lines and a naive line-by-line parse silently truncates it (observed live: a + `--job-name=..._mn_verify` row split across four lines, of which only the first carried the + jobid). Parse continuation-aware: a **new record starts only where a line matches + `^(_)?(\.)?\|`**; any other line is a continuation of the record above it + and must be appended to it. A line-oriented `grep`/`cut` pipeline cannot express this — do it + in a real parser. + - **Do not filter step rows with a bare `grep -v '\.'`.** Submit lines are full of dots (paths, + versions, filenames), so that pattern discards real records — it is the `JobID` field that must + be tested for a `.`, not the whole line. Same mistake as above in a different disguise: both + come from treating the output as text rather than as records. - **Discard step rows before treating a hit as real.** A base job (and every array task within it) returns extra rows for its steps — `.batch`, `.extern`, and any other `.`-suffixed step — with empty `SubmitLine`/`WorkDir`. Drop any returned row whose `JobID`