Merge pull request #406 from xylar/fix-copilot-instruction-formatting

Fix updating config machines (again)

Author: xylar

.github/copilot-instructions.md

@@ -10,6 +10,9 @@ Follow the repository's automated style configuration in
   environment. Prefer `pixi run -e py314 <command>` for `python`, `pytest`,
   `pre-commit`, `ruff`, and `mypy`, and check `.pixi/` plus `pixi.toml`
   before concluding a tool is missing.
+- Do not run `workflow_tests/test_deploy_workflow.py` as part of normal
+  validation. It is a long-running workflow test and should only be run if
+  the user explicitly asks for it.
 - pre-commit on changed files is required before finishing; if sandboxed
   execution fails, request escalation and do not close the task until it has
   run or the user declines.

.github/workflows/cime_machine_config_update.yml

@@ -37,6 +37,7 @@ jobs:
 
       - name: Generate machine update report
         env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
           RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
         run: |
           pixi run -e ${PIXI_ENV} python utils/update_cime_machine_config.py \

AGENTS.md

@@ -43,6 +43,8 @@ These instructions apply to the whole repository unless a deeper
 - Run tests and linting through Pixi unless the task explicitly requires a
   different environment.
 - Prefer `pixi run -e py314 pytest` for tests.
+- Do not run `workflow_tests/test_deploy_workflow.py` during routine
+  validation unless the user explicitly requests that workflow test.
 - pre-commit on changed files is required before finishing; if sandboxed
   execution fails, request escalation and do not close the task until it has
   run or the user declines.

docs/developers_guide/config_machines_updates.md

@@ -142,6 +142,9 @@ The required work section tells Copilot to:
 
 - run `pixi run -e py314 python utils/update_cime_machine_config.py
   --work-dir .`,
+- if GitHub API access is unavailable, rerun the command with
+  `--upstream-revision <sha>` when the upstream E3SM commit is already known,
+  or provide `GITHUB_TOKEN`/`GH_TOKEN` in the environment,
 - replace `mache/cime_machine_config/config_machines.xml` with the generated
   `upstream_config_machines.xml`,
 - remove `upstream_config_machines.xml` before committing,
@@ -231,6 +234,10 @@ pixi run -e py314 python utils/update_cime_machine_config.py \
   --markdown-output /tmp/cime_machine_config_report.md
 ```
 
+If GitHub API access is rate-limited or forbidden in your environment,
+rerun with `GITHUB_TOKEN` or `GH_TOKEN` set, or provide
+`--upstream-revision <sha>` when the upstream commit hash is already known.
+
 Run the focused tests:
 
 ```bash

mache/cime_machine_config/report.py

@@ -133,23 +133,24 @@ def render_update_issue(report, run_url=None):
             'Required work:',
             '',
             '- Run `pixi run -e py314 python '
-            'utils/update_cime_machine_config.py --work-dir .` from the',
-            '  repository root.',
-            '- Replace mache/cime_machine_config/config_machines.xml with',
-            '  upstream_config_machines.xml generated by that command, then',
-            '  remove upstream_config_machines.xml before committing.',
+            'utils/update_cime_machine_config.py --work-dir . '
+            '--upstream-revision <sha>` ',
+            '  from the repository root.',
+            '- Replace `mache/cime_machine_config/config_machines.xml` with',
+            '  `upstream_config_machines.xml` generated by that command, then',
+            '  remove `upstream_config_machines.xml` before committing.',
             '- If module or environment changes imply different',
             '  system-package',
             '  versions, update the corresponding mache Spack templates and',
             '  version strings for the affected toolchains.',
             '- Look for related Spack template files under',
-            '  mache/spack/templates/<machine>*.yaml,',
-            '  mache/spack/templates/<machine>*.sh, and',
-            '  mache/spack/templates/<machine>*.csh.',
+            '  `mache/spack/templates/<machine>*.yaml`,',
+            '  `mache/spack/templates/<machine>*.sh`, and',
+            '  `mache/spack/templates/<machine>*.csh`.',
             '- If the only follow-up is module or version drift, keep the PR',
             '  focused on those updates.',
             '- In the PR summary, state which upstream E3SM commit hash was',
-            '  used for the config_machines.xml replacement.',
+            '  used for the `config_machines.xml` replacement.',
             '- If any prefix or path values changed and the correct',
             '  replacement',
             '  is not obvious, add a TODO comment in the PR for the reviewer',

tests/test_update_cime_machine_config.py

@@ -44,9 +44,10 @@ def raise_for_status(self):
         def json(self):
             return [{'sha': 'abc123'}]
 
-    def fake_get(url, *, params, timeout):
+    def fake_get(url, *, params, headers, timeout):
         captured['url'] = url
         captured['params'] = params
+        captured['headers'] = headers
         captured['timeout'] = timeout
         return FakeResponse()
 
@@ -66,4 +67,82 @@ def fake_get(url, *, params, timeout):
         'path': 'cime_config/machines/config_machines.xml',
         'per_page': 1,
     }
+    assert captured['headers'] == {
+        'Accept': 'application/vnd.github+json',
+        'User-Agent': 'mache/update_cime_machine_config',
+    }
     assert captured['timeout'] == 60
+
+
+def test_get_latest_commit_sha_uses_github_token_when_available(monkeypatch):
+    update_module = _load_update_module()
+    captured = {}
+
+    class FakeResponse:
+        def raise_for_status(self):
+            return None
+
+        def json(self):
+            return [{'sha': 'abc123'}]
+
+    def fake_get(url, *, params, headers, timeout):
+        captured['headers'] = headers
+        return FakeResponse()
+
+    monkeypatch.setenv('GITHUB_TOKEN', 'secret-token')
+    monkeypatch.setattr(update_module.requests, 'get', fake_get)
+
+    sha = update_module._get_latest_commit_sha(
+        owner='E3SM-Project',
+        repo='E3SM',
+        ref='master',
+        path='cime_config/machines/config_machines.xml',
+    )
+
+    assert sha == 'abc123'
+    assert captured['headers'] == {
+        'Accept': 'application/vnd.github+json',
+        'User-Agent': 'mache/update_cime_machine_config',
+        'Authorization': 'Bearer secret-token',
+    }
+
+
+def test_build_report_uses_manual_upstream_revision(monkeypatch):
+    update_module = _load_update_module()
+
+    monkeypatch.setattr(
+        update_module,
+        '_build_report_in_dir',
+        lambda **kwargs: kwargs['upstream_revision'],
+    )
+
+    report = update_module.build_report(
+        upstream_url='https://example.com/config_machines.xml',
+        upstream_revision='deadbeef1234',
+        work_dir='.',
+    )
+
+    assert report == 'deadbeef1234'
+
+
+def test_resolve_upstream_revision_ignores_github_api_errors(monkeypatch):
+    update_module = _load_update_module()
+
+    class FakeResponse:
+        status_code = 403
+
+    def fake_get_latest_commit_sha(**kwargs):
+        raise update_module.requests.HTTPError(response=FakeResponse())
+
+    monkeypatch.setattr(
+        update_module,
+        '_get_latest_commit_sha',
+        fake_get_latest_commit_sha,
+    )
+
+    revision = update_module._resolve_upstream_revision(
+        'https://raw.githubusercontent.com/E3SM-Project/E3SM/'
+        'refs/heads/master/cime_config/machines/config_machines.xml'
+    )
+
+    assert revision is None

utils/README.md

@@ -15,6 +15,10 @@ When drift is detected, the follow-up PR should replace
 from `E3SM-Project/E3SM`. This utility helps confirm the drift and review the
 affected supported machines; it does not rewrite the repository copy in place.
 
+If the script cannot resolve the upstream commit from the GitHub API,
+provide `GITHUB_TOKEN` or `GH_TOKEN` in the environment, or rerun it with
+`--upstream-revision <sha>` when the upstream commit is already known.
+
 The PR should also make the changes associated with the differences that this
 utility displays in the appropriate `mache/spack/templates` files.
 

utils/update_cime_machine_config.py

@@ -2,6 +2,7 @@
 
 import argparse
 import os
+import sys
 import tempfile
 from urllib.parse import urlparse
 
@@ -61,11 +62,17 @@ def main():
         '--work-dir',
         help='Optional directory for temporary XML comparison files.',
     )
+    parser.add_argument(
+        '--upstream-revision',
+        help='Optional upstream revision to record in the report. If omitted, '
+        'the script tries to resolve it from the GitHub API.',
+    )
 
     args = parser.parse_args()
 
     report = build_report(
         upstream_url=args.upstream_url,
+        upstream_revision=args.upstream_revision,
         work_dir=args.work_dir,
     )
     print_console_report(report)
@@ -80,10 +87,11 @@ def main():
             handle.write('\n')
 
 
-def build_report(*, upstream_url, work_dir=None):
+def build_report(*, upstream_url, upstream_revision=None, work_dir=None):
     """Download upstream config and return a structured drift report."""
 
-    upstream_revision = _resolve_upstream_revision(upstream_url)
+    if upstream_revision is None:
+        upstream_revision = _resolve_upstream_revision(upstream_url)
 
     if work_dir is not None:
         return _build_report_in_dir(
@@ -142,12 +150,16 @@ def _resolve_upstream_revision(upstream_url):
         return None
 
     owner, repo, ref, path = github_source
-    return _get_latest_commit_sha(
-        owner=owner,
-        repo=repo,
-        ref=ref,
-        path=path,
-    )
+    try:
+        return _get_latest_commit_sha(
+            owner=owner,
+            repo=repo,
+            ref=ref,
+            path=path,
+        )
+    except requests.RequestException as exc:
+        _warn_revision_resolution_failure(exc)
+        return None
 
 
 def _parse_github_raw_url(upstream_url):
@@ -179,6 +191,7 @@ def _get_latest_commit_sha(*, owner, repo, ref, path):
             'path': path,
             'per_page': 1,
         },
+        headers=_build_github_api_headers(),
         timeout=60,
     )
     response.raise_for_status()
@@ -194,5 +207,31 @@ def _get_latest_commit_sha(*, owner, repo, ref, path):
     return sha
 
 
+def _build_github_api_headers():
+    headers = {
+        'Accept': 'application/vnd.github+json',
+        'User-Agent': 'mache/update_cime_machine_config',
+    }
+
+    token = os.getenv('GITHUB_TOKEN') or os.getenv('GH_TOKEN')
+    if token:
+        headers['Authorization'] = f'Bearer {token}'
+
+    return headers
+
+
+def _warn_revision_resolution_failure(error):
+    response = getattr(error, 'response', None)
+    status_code = getattr(response, 'status_code', None)
+    status = '' if status_code is None else f' (HTTP {status_code})'
+    print(
+        'Warning: could not resolve upstream revision from the GitHub API'
+        f'{status}; continuing without revision metadata. Set '
+        'GITHUB_TOKEN or GH_TOKEN, or rerun with --upstream-revision <sha> '
+        'to record a specific upstream commit.',
+        file=sys.stderr,
+    )
+
+
 if __name__ == '__main__':
     main()