From 77c146455e3d76f2a3f340df5f0536bf1e4174aa Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Thu, 14 May 2026 14:39:26 -0500 Subject: [PATCH 1/4] skill-evolution: tighten PR-description and CI-authoring guidance Add explicit anti-patterns for PR descriptions (no how-it-works walkthroughs, file tables, exhaustive test-plan checklists) and a new "Editing CI scripts and workflows" section covering the reviewer feedback themes from #1194: don't restate framework defaults, no fallback values for required inputs, hard-code GitHub URLs, validate early, split chained bash commands. Signed-off-by: Ramakrishna Prabhu --- skills/cuopt-developer/SKILL.md | 2 +- .../cuopt-developer/resources/contributing.md | 29 ++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/skills/cuopt-developer/SKILL.md b/skills/cuopt-developer/SKILL.md index fde6b17fb1..65237729db 100644 --- a/skills/cuopt-developer/SKILL.md +++ b/skills/cuopt-developer/SKILL.md @@ -202,7 +202,7 @@ cuOpt uses Cython to bridge Python and C++. See [resources/python_bindings.md](r ## Contributing — Commits, PRs, Common Tasks -For pre-commit setup, DCO sign-off (`git commit -s`), the fork-based PR workflow, the draft-PR rule for agents, and step-by-step common-task recipes (adding a solver parameter, dependency, server endpoint, or CUDA kernel), see [resources/contributing.md](resources/contributing.md). +For pre-commit setup, DCO sign-off (`git commit -s`), the fork-based PR workflow, the draft-PR rule for agents, PR-description rules (keep it short — no "how it works" walkthroughs or file tables), CI/workflow authoring conventions (extend existing scripts before adding new ones; no LLM-cruft like restated defaults or fallback values), and step-by-step common-task recipes (adding a solver parameter, dependency, server endpoint, or CUDA kernel), see [resources/contributing.md](resources/contributing.md). ## Coding Conventions diff --git a/skills/cuopt-developer/resources/contributing.md b/skills/cuopt-developer/resources/contributing.md index 7b76ec04d4..1f562b1092 100644 --- a/skills/cuopt-developer/resources/contributing.md +++ b/skills/cuopt-developer/resources/contributing.md @@ -55,7 +55,34 @@ When an AI agent creates a pull request, it **must be a draft PR** (`gh pr creat ### PR Descriptions -Keep PR summaries **short and informative**. State what changed and why in a few bullet points. Avoid verbose explanations, full file listings, or restating the diff. Reviewers read the code — the summary should give them context, not a transcript. +Keep PR summaries **short and informative** — typically a few bullets stating *what* changed and *why*. Most merged cuOpt PRs run a single short paragraph or 3–5 bullets; calibrate by skimming a few recent merges on the target branch before writing. + +**Don't include:** +- "How it works" walkthroughs of the implementation — reviewers read the code. +- File-by-file tables listing every changed path — the diff already shows this. +- Exhaustive test-plan checklists enumerating every assertion. A short high-level test note is fine; a 10-item checklist is not. +- Restating the diff in prose. +- Embedded screenshots of CI dashboards or generated output unless they show something the reviewer can't reproduce locally. + +**Why:** Reviewers skim PR descriptions to get oriented, then read the code. A long, structured summary signals "LLM-generated" and erodes trust in the change. The shorter the summary, the more carefully each line gets read. + +If the change genuinely needs more context (a design decision, an unusual constraint, a follow-up plan), state it in one or two sentences and link to an issue or design doc rather than expanding the PR body. + +### Editing CI scripts and workflows + +CI scripts (`ci/`) and GitHub Actions workflows (`.github/workflows/`) attract LLM-generated cruft more than any other area of the repo because the conventions are unfamiliar and "safe defaults" look helpful. Reviewers push back hard on it. Apply these rules before writing or extending CI: + +- **Prefer extending an existing script or workflow over adding a new one.** New files in `ci/` or `.github/workflows/` need a justification that can't be met by extending what's already there. If you're tempted to add a new file, first identify the closest existing one and explain why it doesn't fit. +- **Every flag, option, and env-var override must trace to a real problem.** If you can't point to the failure mode it prevents, drop it. Reviewers will (and do) ask "is this something you added for a real problem, or LLM-generated?" — assume that question on every line. +- **Don't restate defaults.** GitHub Actions already runs steps with `shell: bash -e {0}`; don't add it explicitly. Same for any framework default — restating it implies the writer thought the default was wrong, which confuses readers. +- **Make interfaces strict; no fallback defaults for required inputs.** If an env var, CLI flag, or workflow input is required, fail loudly when it's missing rather than silently defaulting. The risk of "the job has been silently failing for months" outweighs the convenience of a fallback. +- **Hard-code GitHub-specific URLs.** Use `https://github.com/${GITHUB_REPOSITORY}/...` directly. Don't introduce `${GITHUB_SERVER_URL}` overrides unless cuOpt actually runs on GHES. +- **Validate inputs at the top of the script, before any expensive work.** Argument and env-var checks belong before downloads, S3 calls, or aggregation — surface the misconfiguration fast. +- **Split chained bash commands onto their own lines.** `apt-get update && apt-get install -y curl` reads worse than the two-line form and obscures which command failed when one does. +- **No comments that restate the code.** If a comment would tell a reader something the next line already says, delete it. Reserve comments for the non-obvious *why*. +- **Keep PR-scoped CI additions informational and non-blocking.** A new reporting/aggregation job should not be added to `pr-builder`'s `needs:` list — comment posting and dashboards must not gate merging. + +When in doubt, look at how the surrounding cuOpt scripts handle the same concern and match that style rather than introducing a new convention. ## Common Tasks From 82f78283207cb5894acda4649ed34933ac57c40b Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Thu, 14 May 2026 14:50:13 -0500 Subject: [PATCH 2/4] skill-evolution: tighten and generalize script/CI guidance Shorten the PR-description section, and broaden the CI rules to cover all scripts (shell, Python helpers, workflows) rather than just CI files. Signed-off-by: Ramakrishna Prabhu --- skills/cuopt-developer/SKILL.md | 2 +- .../cuopt-developer/resources/contributing.md | 35 +++++++------------ 2 files changed, 14 insertions(+), 23 deletions(-) diff --git a/skills/cuopt-developer/SKILL.md b/skills/cuopt-developer/SKILL.md index 65237729db..47a2d94b62 100644 --- a/skills/cuopt-developer/SKILL.md +++ b/skills/cuopt-developer/SKILL.md @@ -202,7 +202,7 @@ cuOpt uses Cython to bridge Python and C++. See [resources/python_bindings.md](r ## Contributing — Commits, PRs, Common Tasks -For pre-commit setup, DCO sign-off (`git commit -s`), the fork-based PR workflow, the draft-PR rule for agents, PR-description rules (keep it short — no "how it works" walkthroughs or file tables), CI/workflow authoring conventions (extend existing scripts before adding new ones; no LLM-cruft like restated defaults or fallback values), and step-by-step common-task recipes (adding a solver parameter, dependency, server endpoint, or CUDA kernel), see [resources/contributing.md](resources/contributing.md). +For pre-commit setup, DCO sign-off (`git commit -s`), the fork-based PR workflow, the draft-PR rule for agents, PR-description rules (keep it short — no "how it works" walkthroughs or file tables), script and CI/workflow authoring principles (extend existing files before adding new ones; no speculative flags, restated defaults, or silent fallbacks), and step-by-step common-task recipes (adding a solver parameter, dependency, server endpoint, or CUDA kernel), see [resources/contributing.md](resources/contributing.md). ## Coding Conventions diff --git a/skills/cuopt-developer/resources/contributing.md b/skills/cuopt-developer/resources/contributing.md index 1f562b1092..8c249d333e 100644 --- a/skills/cuopt-developer/resources/contributing.md +++ b/skills/cuopt-developer/resources/contributing.md @@ -55,34 +55,25 @@ When an AI agent creates a pull request, it **must be a draft PR** (`gh pr creat ### PR Descriptions -Keep PR summaries **short and informative** — typically a few bullets stating *what* changed and *why*. Most merged cuOpt PRs run a single short paragraph or 3–5 bullets; calibrate by skimming a few recent merges on the target branch before writing. +Keep summaries short — a paragraph or 3–5 bullets stating *what* and *why*. Skim recent merges on the target branch to calibrate. -**Don't include:** -- "How it works" walkthroughs of the implementation — reviewers read the code. -- File-by-file tables listing every changed path — the diff already shows this. -- Exhaustive test-plan checklists enumerating every assertion. A short high-level test note is fine; a 10-item checklist is not. -- Restating the diff in prose. -- Embedded screenshots of CI dashboards or generated output unless they show something the reviewer can't reproduce locally. +Skip how-it-works walkthroughs, file-by-file tables, exhaustive test-plan checklists, prose restatements of the diff, and screenshots of output the reviewer can reproduce locally. Reviewers read the code; long structured summaries signal LLM-generated and erode trust. -**Why:** Reviewers skim PR descriptions to get oriented, then read the code. A long, structured summary signals "LLM-generated" and erodes trust in the change. The shorter the summary, the more carefully each line gets read. +For extra context (a design decision, unusual constraint, follow-up), one or two sentences with a link to an issue or doc beats expanding the body. -If the change genuinely needs more context (a design decision, an unusual constraint, a follow-up plan), state it in one or two sentences and link to an issue or design doc rather than expanding the PR body. +### Writing scripts and CI workflows -### Editing CI scripts and workflows +Shell scripts, Python helper tools, and CI workflows all attract speculative complexity — flags, fallbacks, and config knobs that "might be useful" but trace back to no real problem. Strip it. -CI scripts (`ci/`) and GitHub Actions workflows (`.github/workflows/`) attract LLM-generated cruft more than any other area of the repo because the conventions are unfamiliar and "safe defaults" look helpful. Reviewers push back hard on it. Apply these rules before writing or extending CI: +- Prefer extending an existing script over adding a new one. If you add a new file, be ready to say why an existing one didn't fit. +- Every flag, option, env-var override, and configuration knob needs a concrete failure mode it prevents. If you can't name one, drop it. +- Don't restate framework or platform defaults — it implies the default was wrong and confuses readers. +- Make required inputs strict: fail loudly when they're missing instead of silently defaulting. Silent fallbacks turn into "this has been broken for months" bugs. +- Validate inputs at the top, before any expensive work, so misconfiguration surfaces fast. +- Keep the source readable: one shell command per line over chained `&&`, and no comments that restate the next line — reserve comments for the non-obvious *why*. +- CI-specific: new informational jobs (reporting, dashboards, comment posting) should not gate merging; keep them out of any required-checks list. -- **Prefer extending an existing script or workflow over adding a new one.** New files in `ci/` or `.github/workflows/` need a justification that can't be met by extending what's already there. If you're tempted to add a new file, first identify the closest existing one and explain why it doesn't fit. -- **Every flag, option, and env-var override must trace to a real problem.** If you can't point to the failure mode it prevents, drop it. Reviewers will (and do) ask "is this something you added for a real problem, or LLM-generated?" — assume that question on every line. -- **Don't restate defaults.** GitHub Actions already runs steps with `shell: bash -e {0}`; don't add it explicitly. Same for any framework default — restating it implies the writer thought the default was wrong, which confuses readers. -- **Make interfaces strict; no fallback defaults for required inputs.** If an env var, CLI flag, or workflow input is required, fail loudly when it's missing rather than silently defaulting. The risk of "the job has been silently failing for months" outweighs the convenience of a fallback. -- **Hard-code GitHub-specific URLs.** Use `https://github.com/${GITHUB_REPOSITORY}/...` directly. Don't introduce `${GITHUB_SERVER_URL}` overrides unless cuOpt actually runs on GHES. -- **Validate inputs at the top of the script, before any expensive work.** Argument and env-var checks belong before downloads, S3 calls, or aggregation — surface the misconfiguration fast. -- **Split chained bash commands onto their own lines.** `apt-get update && apt-get install -y curl` reads worse than the two-line form and obscures which command failed when one does. -- **No comments that restate the code.** If a comment would tell a reader something the next line already says, delete it. Reserve comments for the non-obvious *why*. -- **Keep PR-scoped CI additions informational and non-blocking.** A new reporting/aggregation job should not be added to `pr-builder`'s `needs:` list — comment posting and dashboards must not gate merging. - -When in doubt, look at how the surrounding cuOpt scripts handle the same concern and match that style rather than introducing a new convention. +When in doubt, mirror how the surrounding cuOpt code handles the same concern rather than introducing a new convention. ## Common Tasks From aa79f26a68bd6b474016b3f29538c7e15d2d0cf5 Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Thu, 14 May 2026 16:21:23 -0500 Subject: [PATCH 3/4] skill-evolution: align walkthrough spelling Match the unhyphenated form already used elsewhere in the file. Signed-off-by: Ramakrishna Prabhu --- skills/cuopt-developer/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/cuopt-developer/SKILL.md b/skills/cuopt-developer/SKILL.md index 47a2d94b62..34a2cf0502 100644 --- a/skills/cuopt-developer/SKILL.md +++ b/skills/cuopt-developer/SKILL.md @@ -10,7 +10,7 @@ Contribute to the NVIDIA cuOpt codebase. This skill is for modifying cuOpt itsel **If you just want to USE cuOpt**, switch to the appropriate problem skill (cuopt-routing, cuopt-lp-milp, etc.) -**First-time dev environment setup?** See [resources/first_time_setup.md](resources/first_time_setup.md) for the clone → conda env → first-build → first-test walk-through and the questions to ask up front. +**First-time dev environment setup?** See [resources/first_time_setup.md](resources/first_time_setup.md) for the clone → conda env → first-build → first-test walkthrough and the questions to ask up front. --- From af49bc08955c3593e546771f342e178b822ae917 Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Fri, 15 May 2026 11:16:53 -0500 Subject: [PATCH 4/4] skill-evolution: anchor script/CI guidance on YAGNI Per review: collapse the speculative-complexity bullets into a single YAGNI rule (scoped to scripts and CI, not the whole repo), keep only the non-YAGNI points (extend existing files, validate early, readable shell, non-blocking CI jobs). Signed-off-by: Ramakrishna Prabhu --- .../cuopt-developer/resources/contributing.md | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/skills/cuopt-developer/resources/contributing.md b/skills/cuopt-developer/resources/contributing.md index 8c249d333e..c904c0cc09 100644 --- a/skills/cuopt-developer/resources/contributing.md +++ b/skills/cuopt-developer/resources/contributing.md @@ -63,17 +63,16 @@ For extra context (a design decision, unusual constraint, follow-up), one or two ### Writing scripts and CI workflows -Shell scripts, Python helper tools, and CI workflows all attract speculative complexity — flags, fallbacks, and config knobs that "might be useful" but trace back to no real problem. Strip it. +Follow YAGNI strictly here — flags, fallbacks, env-var overrides, and config knobs without a concrete failure mode they prevent should be dropped. This applies to scripts and CI workflows specifically, not the codebase as a whole. -- Prefer extending an existing script over adding a new one. If you add a new file, be ready to say why an existing one didn't fit. -- Every flag, option, env-var override, and configuration knob needs a concrete failure mode it prevents. If you can't name one, drop it. -- Don't restate framework or platform defaults — it implies the default was wrong and confuses readers. -- Make required inputs strict: fail loudly when they're missing instead of silently defaulting. Silent fallbacks turn into "this has been broken for months" bugs. -- Validate inputs at the top, before any expensive work, so misconfiguration surfaces fast. -- Keep the source readable: one shell command per line over chained `&&`, and no comments that restate the next line — reserve comments for the non-obvious *why*. -- CI-specific: new informational jobs (reporting, dashboards, comment posting) should not gate merging; keep them out of any required-checks list. +A few non-YAGNI points worth keeping in mind: -When in doubt, mirror how the surrounding cuOpt code handles the same concern rather than introducing a new convention. +- Prefer extending an existing script over adding a new one. +- Validate inputs at the top, before any expensive work. +- One shell command per line over chained `&&`; no comments that restate the next line. +- Keep informational CI jobs (reporting, dashboards, comment posting) out of any required-checks list. + +When in doubt, mirror how the surrounding cuOpt code handles the same concern. ## Common Tasks