Repository navigation
Conversation
|
You can preview documentation at https://esmci.github.io/cime/branch/docs/agent-guidance-pr/html/index.html |
|
@jasonb5 , can you add a little to the description as to why this new tool is needed? |
There was a problem hiding this comment.
🟡 Changes recommended
Backend selection, Podman permissions, and concurrent cache publication have unresolved correctness issues.
3 open issues
What changed in this PR
Adds a portable pytest runner for CIME’s development workflow.
Changes:
- Supports host and container execution, combined coverage, cached input data, and retained artifacts.
- Adds runner unit tests and usage documentation.
- Updates agent guidance and Claude testing skills.
Validation here was limited to source inspection; tests and builds were not run. Please review and understand the complete changes before requesting review.
| File | Description |
|---|---|
doc/source/contributing-guide.rst |
Documents runner usage and batch approval. |
devtools/run_tests |
Implements backend selection and test execution. |
CIME/tests/test_unit_run_tests.py |
Tests runner behavior and safety checks. |
AGENTS.md |
Expands development and validation guidance. |
.gitignore |
Allows tracked Claude testing skills. |
.claude/skills/cime-unit-testing/SKILL.md |
Routes Claude to shared unit-test guidance. |
.claude/skills/cime-sys-testing/SKILL.md |
Routes Claude to shared system-test guidance. |
.agents/skills/cime-unit-testing/SKILL.md |
Recommends runner commands and failure handling. |
.agents/skills/cime-sys-testing/SKILL.md |
Updates system-test workflow and batch safeguards. |
Review effort: Balanced
|
@jasonb5 I'll be happy to look this over once you check the "I personally reviewed and understand the complete set of changes in this PR" and "I... can explain their purpose, correctness, and validation" boxes. For now, I'm dismissing my review request. |
|
@jgfouca Updated the PR description. |
samsrabin
left a comment
There was a problem hiding this comment.
Thanks for this, Jason! The testing wrapper is a great idea. I've added some requests and comments inline; here are more general ones:
- Should we get rid of
.claude/CLAUDE.md, since Claude now looks inAGENTS.mdas well? - Have you tested all three paths of the wrapper? I.e. (1) supported machine, (2) container, (3) generic machine?
Note that I haven't yet looked at devtools/run_tests or its test file.
| Use the repository runner for CIME's pytest system suite. If it selects a | ||
| batch-enabled host, ask the user whether batch submission is approved; only | ||
| pass `--allow-batch` after approval. Otherwise, choose a container backend or | ||
| allow the runner to stop with its batch-safety message. |
There was a problem hiding this comment.
Might want to add a note saying that each approval is only valid once; agents shouldn't take one approval as permission to submit subsequent batch jobs without approval.
There was a problem hiding this comment.
This is reasonable, I'll update the text.
There was a problem hiding this comment.
I think it would be better to have just one unified cime-testing skill for both unit and system tests. That will slightly reduce token usage, but more importantly it will be easier for us to maintain.
There was a problem hiding this comment.
The original goal was to keep them separate and targeted. I think I'm fine combining them, reviewing them again I don't think I have a strong enough argument to keep them separate.
|
|
||
| ### Run tests | ||
|
|
||
| Prefer the repository runner, which selects a suitable host or container and |
There was a problem hiding this comment.
Inconsistency between here ("prefer") and the system-testing skill ("use").
There was a problem hiding this comment.
There are diffs between this file and its equivalent in .agents/.
There was a problem hiding this comment.
There are diffs between this file and its equivalent in .agents/.
| ## Commits and PRs | ||
|
|
||
| - Use Conventional Commits standard for PR titles, and commit messages. | ||
| - Commit or push changes only when the user explicitly asks. |
There was a problem hiding this comment.
I think "Commit or push changes only when the user explicitly asks" verges more toward "personal preference" as opposed to "we don't want this happening" and thus should be removed. I personally prefer LLMs to commit their changes so I have a clean repo to look at. I also don't have any problem with someone letting their agents push (especially to forks), although I don't let them myself.
There was a problem hiding this comment.
Fair point, I'm fine removing this.
| - When a test fails, verify the intended behavior before changing code or tests. Fix the implementation when it violates the requirement. Change a test only when the test is incorrect or the intended behavior has deliberately changed, and explain why. Do not weaken or rewrite tests just to make a failing implementation pass. | ||
| - Use pytest for new test modules. When adding tests to an existing module, follow that module's established style, including `unittest.TestCase` if it already uses it. Don't convert an existing module or mix styles just to follow this preference. If the appropriate style is unclear, ask the user how to proceed. | ||
| - Run CIME pytest tests with `./devtools/run_tests unit`, `./devtools/run_tests system`, or both suites with `./devtools/run_tests all`; pass multiple pytest targets after `--` to run a subset. | ||
| - Before running system tests on a batch-enabled host, ask the user whether batch submission is approved. Use `--allow-batch` only after approval. |
There was a problem hiding this comment.
I would try to avoid duplicating information between here and the skills, as it's a maintainability burden. When AGENTS.md and skills files differ, it's not clear how LLMs will behave.
There was a problem hiding this comment.
I'll make the change and consolidate everything test related into the skill and just reference the skill from the AGENTS.md file.
| @@ -40,5 +40,11 @@ data/ | |||
| # Special for Claude Code user config | |||
There was a problem hiding this comment.
Is the purpose of this section to allow people to write their own custom Claude skills specifically for working with CIME that they don't want to contribute back? Questions on that:
- Why would we want to make that easy?
- If keeping, shouldn't there be an equivalent for
.agents/skills/?

Description
The purpose of this PR is to introduce developer tooling/guidance to improve using AI agents to develop and test CIME code. The skills and wrapper aim to simplify how an agent runs and analyze tests. The guidance improves the agents role in the development workflow.
The skills and wrapper work together to reduce the guess work an agent needs to do in order to run tests. The previous skills required docker to be installed, the new skills with the wrapper remove this requirement. The agent no longer needs to determine how to run the tests only which tests it needs to run.
The wrapper checks for a valid model checkout, then it will choose the appropriate method to run the tests, this differs depending on
unitorsystests and which tools are available on the system. It also handles using cached input data similar to the CI workflow, preserving tests outputs for the agent to review. The wrapper can also be used by developers as a quick simple way to invoke tests. If runningsystests on an unsupported machine, the wrapper will fall back todockerorpodmanif available.AI agent guidance has been updated with the following:
modelto beEarth system modelpre-commit, use issue/pr templates, must present draft before opening issue or PR, clarify which validations were run by the agentAI Assistance
See the AI usage and human review policy.
No generative AI was used in preparing this contribution.
Generative AI was used; details are below.
AI usage: Assisted with code, tests, documentation, review, and validation.
Provider: GitHub Copilot
Model:
github-copilot/gpt-6-lunaChecklist
Validation: Runner unit tests and pre-commit passed locally. The full combined suite was not run locally; CI results are tracked in this PR.