From 2a74ff624b28060a78c4a7fb3703f129b4cc4fc2 Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Mon, 10 Aug 2026 15:10:14 +0200 Subject: [PATCH 1/4] Add weekly review pipeline --- .github/workflows/icon4py-weekly-review.yml | 66 +++ .github/workflows/weekly-slack-summary.yml | 35 +- .gitignore | 3 + README.md | 3 + content/index.md | 4 + content/review/index.md | 23 + content/review/issues/.gitkeep | 0 content/review/reports/.gitkeep | 0 scripts/icon4py-review/README.md | 100 ++++ scripts/icon4py-review/SKILL.md | 191 +++++++ .../agents/icon4py-correctness-reviewer.md | 81 +++ .../agents/icon4py-finding-skeptic.md | 66 +++ .../agents/icon4py-fixedness-checker.md | 35 ++ .../agents/icon4py-performance-reviewer.md | 77 +++ scripts/icon4py-review/collect-open-issues.py | 93 ++++ scripts/icon4py-review/commit-and-pr.sh | 57 +++ .../icon4py-review/extract-report-commit.py | 59 +++ scripts/icon4py-review/models.json | 18 + scripts/icon4py-review/reconcile-issues.py | 471 ++++++++++++++++++ scripts/icon4py-review/run.sh | 145 ++++++ scripts/icon4py-review/settings.json | 7 + scripts/icon4py-review/update-index.py | 123 +++++ scripts/icon4py-review/validate-issues.py | 154 ++++++ scripts/pi-sandboxed.sh | 147 ++++++ scripts/post-slack-summary/run.sh | 60 +++ 25 files changed, 1999 insertions(+), 19 deletions(-) create mode 100644 .github/workflows/icon4py-weekly-review.yml create mode 100644 content/review/index.md create mode 100644 content/review/issues/.gitkeep create mode 100644 content/review/reports/.gitkeep create mode 100644 scripts/icon4py-review/README.md create mode 100644 scripts/icon4py-review/SKILL.md create mode 100644 scripts/icon4py-review/agents/icon4py-correctness-reviewer.md create mode 100644 scripts/icon4py-review/agents/icon4py-finding-skeptic.md create mode 100644 scripts/icon4py-review/agents/icon4py-fixedness-checker.md create mode 100644 scripts/icon4py-review/agents/icon4py-performance-reviewer.md create mode 100755 scripts/icon4py-review/collect-open-issues.py create mode 100755 scripts/icon4py-review/commit-and-pr.sh create mode 100755 scripts/icon4py-review/extract-report-commit.py create mode 100644 scripts/icon4py-review/models.json create mode 100755 scripts/icon4py-review/reconcile-issues.py create mode 100755 scripts/icon4py-review/run.sh create mode 100644 scripts/icon4py-review/settings.json create mode 100755 scripts/icon4py-review/update-index.py create mode 100755 scripts/icon4py-review/validate-issues.py create mode 100755 scripts/pi-sandboxed.sh create mode 100755 scripts/post-slack-summary/run.sh diff --git a/.github/workflows/icon4py-weekly-review.yml b/.github/workflows/icon4py-weekly-review.yml new file mode 100644 index 0000000..6ec5acb --- /dev/null +++ b/.github/workflows/icon4py-weekly-review.yml @@ -0,0 +1,66 @@ +name: Weekly icon4py review + +on: + schedule: + # Sunday at 22:00 Europe/Zurich + - cron: "0 22 * * 0" + timezone: "Europe/Zurich" + workflow_dispatch: + # TODO: Remove this before merge. + pull_request: + +permissions: + contents: write + pull-requests: write + +concurrency: + group: icon4py-weekly-review + cancel-in-progress: false + +env: + TZ: Europe/Zurich + +jobs: + review: + runs-on: ubuntu-latest + timeout-minutes: 180 + steps: + - name: Checkout icon4py-knowledge + uses: actions/checkout@v4 + + - name: Checkout icon4py + uses: actions/checkout@v4 + with: + repository: C2SM/icon4py + ref: main + path: icon4py-checkout + fetch-depth: 1 + persist-credentials: false + + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y bubblewrap + + - name: Install uv + uses: astral-sh/setup-uv@v6 + + - name: Install Node.js + uses: actions/setup-node@v4 + with: + node-version: '22.19' + + - name: Install pi + run: npm install -g @earendil-works/pi-coding-agent@0.80.6 + + - name: Smoke test + run: | + pi --version + bwrap --version | head -1 + + - name: Run review + env: + CSCS_INFERENCE_API_KEY: ${{ secrets.CSCS_INFERENCE_API_KEY }} + ICON4PY_CHECKOUT: ${{ github.workspace }}/icon4py-checkout + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: ./scripts/icon4py-review/run.sh --commit-and-pr diff --git a/.github/workflows/weekly-slack-summary.yml b/.github/workflows/weekly-slack-summary.yml index 5c2bc49..9a37d2a 100644 --- a/.github/workflows/weekly-slack-summary.yml +++ b/.github/workflows/weekly-slack-summary.yml @@ -9,6 +9,8 @@ on: permissions: contents: read + issues: read + pull-requests: read jobs: weekly-summary: @@ -18,32 +20,27 @@ jobs: - name: Checkout repository uses: actions/checkout@v4 - - name: Install uv - uses: astral-sh/setup-uv@v7 + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y bubblewrap + + - name: Install Node.js + uses: actions/setup-node@v4 with: - enable-cache: true + node-version: '22.19' - name: Install pi - run: npm install -g --ignore-scripts @earendil-works/pi-coding-agent + run: npm install -g @earendil-works/pi-coding-agent@0.80.6 - - name: Configure pi for CSCS inference + - name: Smoke test run: | - mkdir -p ~/.pi/agent - cp .github/workflows/weekly-slack-summary/settings.json ~/.pi/agent/settings.json - cp .github/workflows/weekly-slack-summary/models.json ~/.pi/agent/models.json + pi --version + bwrap --version | head -1 - - name: Generate weekly summary + - name: Generate and post summary env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} CSCS_INFERENCE_API_KEY: ${{ secrets.CSCS_INFERENCE_API_KEY }} - run: | - pi -p --approve \ - --tools read,bash,edit,write,grep,find,ls \ - --skill .github/workflows/weekly-slack-summary \ - "Generate the weekly Slack activity summary for icon4py-knowledge and write it to weekly_slack_summary.md" - test -s weekly_slack_summary.md - - - name: Post summary to Slack - env: SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} - run: ./scripts/post-slack-summary.py weekly_slack_summary.md + run: ./scripts/post-slack-summary/run.sh diff --git a/.gitignore b/.gitignore index d2a56e2..532b818 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,9 @@ *.local.md .claude/settings.local.json +# Python cache +__pycache__/ + # Obsidian .obsidian/ diff --git a/README.md b/README.md index ea0f05e..377f670 100644 --- a/README.md +++ b/README.md @@ -53,3 +53,6 @@ cd /tmp/quartz-preview && npx quartz build --serve # http://localhost:8080 - **Weekly Slack summary**:— `.github/workflows/weekly-slack-summary.yml` posts a Monday-morning summary of the last calendar week's activity to Slack. +- **Weekly icon4py review**: `.github/workflows/icon4py-weekly-review.yml` runs an + automated review of C2SM/icon4py and opens a PR with accepted + findings. See [`scripts/icon4py-review/README.md`](scripts/icon4py-review/README.md). diff --git a/content/index.md b/content/index.md index ec5beeb..c91d7bf 100644 --- a/content/index.md +++ b/content/index.md @@ -10,6 +10,10 @@ topics a document discusses — scan them to find overlapping or conflicting ide See `AGENTS.md` in the repository root for how to add a proposal and keep this index current. (Keep entries and their keywords in sync with each document's `tags`.) +## Automated review tracker + +- [[review/index|Weekly icon4py review tracker]] — keywords: review, correctness, performance, issues + ## Shared Proposals the group broadly agrees are implementation-ready. diff --git a/content/review/index.md b/content/review/index.md new file mode 100644 index 0000000..66dd8c7 --- /dev/null +++ b/content/review/index.md @@ -0,0 +1,23 @@ +--- +title: Automated icon4py review tracker +--- + + + +This page tracks automated review findings for C2SM/icon4py. Individual issue files live in `review/issues/`. Weekly overview reports live in `review/reports/`. + +## Open issues + +_No open issues._ + +## Fixed issues + +_No fixed issues._ + +## Invalid issues + +_No invalid issues._ + +## Reports + +_No reports._ diff --git a/content/review/issues/.gitkeep b/content/review/issues/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/content/review/reports/.gitkeep b/content/review/reports/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/scripts/icon4py-review/README.md b/scripts/icon4py-review/README.md new file mode 100644 index 0000000..9650b2c --- /dev/null +++ b/scripts/icon4py-review/README.md @@ -0,0 +1,100 @@ +# icon4py weekly review + +An automated weekly review of [C2SM/icon4py](https://github.com/C2SM/icon4py). +It runs on a schedule in CI and can be run locally for testing. + +The review is currently limited to performance and correctness issues, +high-severity issues, and will only be reported if all three of a three-member +skeptic panel verifies the issue as real. This is tuned to avoid reduce noise +from too many issues being reported and to avoid false positives. This can be +tuned over time to include lower severity issues and different types of issues. + +## What it does + +Each run reviews the icon4py checkout at a given commit and writes accepted +findings as issue files under `content/review/issues/`, plus an overview report +under `content/review/reports/`. The workflow is defined in +[`SKILL.md`](SKILL.md) and executed by an isolated `pi` orchestrator: + +1. **Reviewers.** `icon4py-correctness-reviewer` and + `icon4py-performance-reviewer` search the checkout. They read + `open-issues.json` (existing issues, so they do not re-report them) and + `changes.diff` (code changed since the last review, so they prioritize it). +2. **Skeptic panel.** Each candidate finding gets three `icon4py-finding-skeptic` + votes. A finding is accepted only if all three vote `PASS`; any `REJECT` + rejects it; matching `DUPLICATE` votes merge it into an existing issue. +3. **Fixedness.** Existing open issues not matched by a new finding get three + `icon4py-fixedness-checker` votes (`fixed`, `persists`, `unknown`) to track + whether known problems are still present. +4. **Reconcile and publish.** `reconcile-issues.py` writes accepted findings as + issue files and updates fixedness. `update-index.py` regenerates + `content/review/index.md`. + +``` + host (run.sh) sandbox (pi orchestrator) + -------------- ---------------------- + open-issues.json -------------+-> reviewers --> correctness.json + changes.diff -----------------+ performance.json + | + v + finding-skeptic x3 --> votes/.../*.json + | + v + fixedness-checker x3 (only issues not matched + by a new finding, with a changed commit) + | + v + accepted.json duplicates.json fixedness.json + <----------- overview report <---------- + + reconcile-issues.py --> content/review/issues/.md + update-index.py --> content/review/index.md +``` + +Reviewers and checkers cannot run `git` or `bash`: they run in a `bwrap` +sandbox with a read-only checkout and a narrow writable findings directory. The +host generates `open-issues.json` and `changes.diff` and places them where the +sandbox can read them. + +## Inputs and secrets + +- `CSCS_INFERENCE_API_KEY`: API key for the inference provider. Required. +- `GITHUB_TOKEN`: used only by `commit-and-pr.sh`, outside the sandbox, to open + the pull request. Not forwarded into the sandbox. +- `ICON4PY_CHECKOUT`: absolute path to a clone of `C2SM/icon4py`. Required for + local runs; set by the workflow in CI. + +## Run locally + +```bash +ICON4PY_CHECKOUT=/path/to/icon4py ./scripts/icon4py-review/run.sh +``` + +This produces changes for inspection without committing. Add `--commit-and-pr` +to commit the result and open a pull request (requires `gh` and `GITHUB_TOKEN`). + +## Files + +``` +.github/workflows/ + icon4py-weekly-review.yml # the GitHub Actions workflow +scripts/icon4py-review/ + run.sh # entry point: collects inputs, runs the sandbox, + # reconciles, validates, regenerates the index + SKILL.md # the orchestrator workflow definition + agents/ # correctness, performance, skeptic, fixedness + settings.json # pi provider and package config + models.json # model definitions (keys come from env) + README.md # this document + collect-open-issues.py # writes open-issues.json from content/review/issues/ + extract-report-commit.py # reads the previous report's icon4py_commit + reconcile-issues.py # writes accepted issues and fixedness verdicts + validate-issues.py # checks issue files are well-formed + update-index.py # regenerates content/review/index.md + commit-and-pr.sh # commits the result and opens a PR (CI only) +scripts/pi-sandboxed.sh # bwrap sandbox wrapper for pi +``` + +Each review report records its `icon4py_commit` in the frontmatter. The next run +uses that commit as the diff baseline, so review attention moves forward with +the code while existing issues are tracked separately for fixedness. diff --git a/scripts/icon4py-review/SKILL.md b/scripts/icon4py-review/SKILL.md new file mode 100644 index 0000000..1f46150 --- /dev/null +++ b/scripts/icon4py-review/SKILL.md @@ -0,0 +1,191 @@ +--- +name: icon4py-weekly-review +description: "Run the weekly automated review of C2SM/icon4py and write accepted findings and a fixedness assessment." +--- + +# Weekly icon4py review + +You are the orchestrator. Spawn read-only subagents to review the code and +verify findings, then write three JSON files to the findings directory and one +overview report to the reports directory. + +## What your task prompt provides + +- `icon4py_checkout`: absolute path to the icon4py clone (read-only). +- `icon4py_commit`: the commit being reviewed. +- `review_date`: `YYYY-MM-DD` (report filename, issue metadata). +- `run_id`: the run identifier (history entries). +- `requested_severity`: tell reviewers to report only this severity. +- `findings_dir`: writable directory for your JSON outputs. +- `reports_dir`: writable directory for the overview report. + +Pass these values to each subagent in its task prompt. + +Existing open issues are in `/open-issues.json`, a flat list with +one entry each: + +```json +[{"id": "icon4py-...", "fingerprint": "...", "file": "", "commit_sha": "...", "title": "...", "description": "...", "suggested_fix": "...", "lines": [s,e], "symbol": "..."}] +``` + +## Thresholds + +- Acceptance: a finding becomes a **new** issue only if all three skeptics vote + `PASS`. A valid finding that duplicates an existing issue is merged into that + issue instead. Any `UNCERTAIN` vote prevents a new issue from being created. +- Fixedness: an existing open issue is marked `fixed` only if at least two of + the three fixedness checkers vote `fixed`. Otherwise the verdict is the + majority of the three votes, or `unknown` if there is no majority, and the + issue stays open. + +## Step 1: Run the reviewers + +Run both reviewers and collect all findings before continuing: + +- `icon4py-correctness-reviewer`, output `/correctness.json` +- `icon4py-performance-reviewer`, output `/performance.json` + +Each task prompt gives `icon4py_checkout`, `requested_severity`, `output_path`, +and `findings_dir`. Pass `findings_dir` so reviewers can read `open-issues.json` +(already-tracked issues to avoid re-reporting) and `changes.diff` (code changed +since the last review) from there. + +If a reviewer fails or its output is missing or malformed, record the failure +in the overview report and treat that reviewer's findings as empty. Continue. + +## Step 2: Verify findings with the skeptic panel + +Combine all findings. For each finding, run three `icon4py-finding-skeptic` +subagents. Each task prompt gives: +- `icon4py_checkout` +- the full `finding` object +- `open_issues`: the list from `/open-issues.json` +- `output_path` under `/votes//1.json`, `2.json`, `3.json` + +Each skeptic returns `verdict` in `PASS|REJECT|DUPLICATE|UNCERTAIN` and, +for `DUPLICATE`, a `duplicate_of` issue id. + +A missing or malformed vote counts as `UNCERTAIN`. + +Classify each finding based on the three verdicts, in this order: + +- **Rejected**: any verdict is `REJECT`. Record in the report; do not create an issue. +- **New issue** (`accepted.json`): all three verdicts are `PASS`. +- **Duplicate merge** (`duplicates.json`): at least one verdict is `DUPLICATE` + and the others are `PASS`. All `DUPLICATE` votes must name the same + `duplicate_of` issue id; split targets are uncertain, not a merge. + `confidence` is `high` for 3x `DUPLICATE`, `medium` for 2x, `low` for 1x + `DUPLICATE` with the other two `PASS`. +- **Uncertain/ambiguous**: anything else (e.g., `UNCERTAIN`, split `DUPLICATE` + targets, or `PASS` mixed with `UNCERTAIN`). Record these in the report but do + not create an issue. + +Record missing or malformed votes and the classification of each finding in +the overview report. + +## Step 3: Write accepted.json and duplicates.json + +### 3.1 accepted.json + +Write truly new findings to `/accepted.json` as a flat JSON list. +Each entry is the reviewer's raw finding object (fingerprint, title, severity, +description, evidence, file, lines, symbol, suggested_fix) enriched by the +orchestrator with `reviewer` (the reviewer name), `confidence` (`high`, from +all-3-PASS), and `tags` (derived from the title and defect type; format below). +Skip a malformed entry with a note in the overview report rather than aborting. + +`tags` is a list of 3-6 short kebab-case keywords that help someone scan the +tracker for related issues. Do not include the reviewer name, the date, or any +string longer than a short phrase. + +### 3.2 duplicates.json + +Write findings that duplicate an existing open issue to `/duplicates.json` as a flat list: + +```json +[ + { + "finding": {... accepted finding object ...}, + "duplicate_of": "icon4py-YYYY-MM-DD-XXXXXXX", + "duplicate_of_fingerprint": "...", + "confidence": "high|medium|low", + "reasoning": "..." + } +] +``` + +`confidence` follows the duplicate-merge classification above: `high` for 3x +`DUPLICATE`, `medium` for 2x, `low` for 1x `DUPLICATE` with the others `PASS`. +Any other case is not a duplicate merge; record it as uncertain in the report. + +## Step 4: Assess fixedness of existing open issues + +Let `accepted_fingerprints` be the set of fingerprints in `accepted.json`. Let +`duplicate_matched_existing` be the set of `duplicate_of_fingerprint` values in +`duplicates.json`. + +For each entry in `open-issues.json` whose fingerprint is NOT in +`accepted_fingerprints` AND NOT in `duplicate_matched_existing`: + +- If the entry's `commit_sha` equals `icon4py_commit` (the reviewers saw the + same code and did not reproduce it), write `{"fingerprint": "...", "verdict": "not-detected"}` + directly. Do not spawn a panel. +- If the commit changed, run three `icon4py-fixedness-checker` subagents + against `/`. Each task prompt gives + `icon4py_checkout`, the issue (id, title, description, file, lines, symbol, + suggested_fix), and an `output_path`. + + **Merge the three verdicts into exactly one entry per issue.** Apply this + fixedness threshold: + - `fixed`: at least two of the three votes are `fixed`. + - `persists`: at least two of the three votes are `persists`. + - `unknown`: no majority (e.g., one vote each, or two `unknown`). + A missing or malformed vote counts as `unknown`. + +Write `/fixedness.json` as a flat list with **exactly one entry +per unmatched open issue**: + +```json +[{"fingerprint": "...", "verdict": "fixed|persists|unknown|not-detected"}] +``` + +Do not write one entry per checker vote. If an issue has three checker +verdicts, they must be merged into a single entry before writing the file. + +A missing entry for an unmatched open issue leaves it open with a warning. +`unknown` means the panel could not decide (possibly stale) and leaves the +issue open. + +## Step 5: Write the overview report + +Write the overview document to `/-.md` +(e.g. `2026-08-05-2114.md`) so same-day runs do not conflict. The report is +Markdown with this frontmatter: + +```yaml +--- +title: "Weekly icon4py review " +tags: +- review +created: +icon4py_commit: +--- +``` + +The `icon4py_commit` frontmatter field lets the next run's `run.sh` compute a +diff baseline from this report. Put the same commit in the body metadata too. + +The body includes: + +- Run metadata: date, run id, icon4py commit. +- Summary counts: total findings reviewed, findings submitted to the panel, + new accepted findings, merged duplicates, rejected findings, uncertain + findings, and the fixedness outcome for existing open issues. +- A table of new accepted findings keyed by `fingerprint`, with title, + severity, confidence, and file. +- A table of merged duplicates: new finding title, existing issue id, and + confidence. +- A table of rejected findings with title, the skeptic verdicts, and the file. +- A table of uncertain/ambiguous findings with title, verdicts, and note. +- A table of fixedness outcomes: issue id, verdict, and a note. +- Notes on any reviewer, skeptic, or fixedness failures. diff --git a/scripts/icon4py-review/agents/icon4py-correctness-reviewer.md b/scripts/icon4py-review/agents/icon4py-correctness-reviewer.md new file mode 100644 index 0000000..8fa5a3c --- /dev/null +++ b/scripts/icon4py-review/agents/icon4py-correctness-reviewer.md @@ -0,0 +1,81 @@ +--- +name: icon4py-correctness-reviewer +description: Review C2SM/icon4py for correctness issues at the requested severity. +prompt_mode: replace +tools: read, grep, find, ls, write, edit +--- + +You are a specialist reviewer for the C2SM/icon4py climate/weather model +codebase. Find correctness issues: bugs that can cause wrong results, crashes, +silent data corruption, or non-deterministic behavior in production runs. + +Your task prompt gives you `icon4py_checkout` (an absolute path to the clone), +`requested_severity` (only look for and report issues with the given or more severe +severity), `output_path` (where to write the JSON findings file), and +`findings_dir` (writable directory shared with the orchestrator). + +Severity levels: +- `high`: likely to cause wrong results, crashes, silent data corruption, or + non-determinism in production runs. +- `medium`: a potential issue, or code that is fragile or misleading, that + could cause incorrect behavior under conditions not exercised in production. +- `low`: an unlikely edge case or minor robustness issue with no practical + impact on production runs. + +Before reviewing, read the icon4py AGENTS.md at `/AGENTS.md` +and any referenced coding-guideline files for the codebase conventions. + +Start by reading `/open-issues.json` so you do not re-report +issues that are already tracked. You may still examine the same files for other +issues. + +Then read `/changes.diff` for the code that changed since the last +review. Prioritize the changed code, but also examine the rest of the checkout. +If `changes.diff` is absent, review the full checkout. + +Correctness issues include, but are not limited to: + +- GT4Py stencil domain mismatches or unsafe offset/Connectivity reads. +- Out-of-bounds or skip-value reads from connectivities or K-level offsets. +- MPI correctness problems: tag collisions, missing synchronization, non-matching send/recv, reductions over wrong communicators. +- Fortran binding mismatches in py2fgen wrappers (shape, order, intent, lifetime). +- Numerical determinism problems: order-sensitive reductions, rank-dependent floating-point paths. + +Do not report style issues, minor refactors, or speculative GPU race conditions +in generated kernels. Security issues are out of scope unless they directly +affect model correctness. + +Write a single valid JSON file to `output_path` with exactly this shape: + +```json +{ + "reviewer": "icon4py-correctness-reviewer", + "findings": [ + { + "fingerprint": "correctness:::", + "title": "...", + "severity": "high", + "description": "...", + "evidence": "...", + "file": "", + "lines": [start, end], + "symbol": "...", + "suggested_fix": "..." + } + ] +} +``` + +`description`, `evidence`, and `suggested_fix` are inline Markdown (no headings): +use backticks for code, symbols, and file paths; `*` for emphasis; lists where +useful. The issue file supplies its own `## Summary` / `## Evidence` / `## +Suggested fix` headings; do not include headings in these field values. + +`tags` is a list of 3-6 short kebab-case keywords that help someone scan the +tracker for related issues (e.g. `mpi`, `gpu`, `halo-exchange`, `memory`). Do +not include the reviewer name, the date, or any string longer than a short +phrase. + +Fingerprints must be stable across runs: reviewer prefix, relative file path, symbol or line anchor, and defect type. Do not include the title. + +If there are no findings at the requested severity, write `{"reviewer": "icon4py-correctness-reviewer", "findings": []}`. diff --git a/scripts/icon4py-review/agents/icon4py-finding-skeptic.md b/scripts/icon4py-review/agents/icon4py-finding-skeptic.md new file mode 100644 index 0000000..a1405d0 --- /dev/null +++ b/scripts/icon4py-review/agents/icon4py-finding-skeptic.md @@ -0,0 +1,66 @@ +--- +name: icon4py-finding-skeptic +description: Validates a proposed finding and checks whether it duplicates an existing open issue. +prompt_mode: replace +tools: read, grep, find, ls, write, edit +--- + +Your job is to assess a proposed finding and decide whether it should become a +new tracker issue, merge into an existing open issue, or be rejected. + +Your task prompt gives you: +- `icon4py_checkout`: absolute path to the icon4py clone (read-only). +- `finding`: the full finding object (title, description, file, lines, symbol, + evidence, suggested_fix). +- `open_issues`: a flat list of existing open issues, each with `id`, + `fingerprint`, `file`, `lines`, `symbol`, `title`, `description`, and + `suggested_fix`. +- `output_path`: where to write your verdict JSON. + +## Step 1: Validate the finding + +Read the exact source location at `/` around +``. Use `grep` and `find` to check surrounding code, tests, +comments, and conventions that bear on the finding. + +Decide whether the finding is accurate, reachable, and of the given severity level. + +## Step 2: Check for duplicates + +Compare the finding against every entry in `open_issues`. Consider whether they +describe the same underlying problem, even if the titles, symbols, line ranges, +or suggested fixes differ. Two findings about the same code location and root +cause are duplicates. + +## Step 3: Vote + +Vote exactly one of: + +- `PASS`: the finding is valid and **not** a duplicate of any existing open + issue. +- `REJECT`: the finding is inaccurate, unreachable, already mitigated, + exaggerated, or not actionable. +- `DUPLICATE`: the finding is valid, but it describes the same underlying issue + as an existing open issue. Set `duplicate_of` to the existing issue `id`. +- `UNCERTAIN`: you cannot confidently decide validity or duplication. + +**Important:** Only vote `PASS` if you are confident the finding is both + accurate and not already tracked. When in doubt, vote `DUPLICATE` (if it + clearly overlaps an existing issue) or `UNCERTAIN`. Do not vote `PASS` just + because you are unsure. + +Write a JSON file to `output_path`: + +```json +{ + "voter": "icon4py-finding-skeptic", + "verdict": "PASS|REJECT|DUPLICATE|UNCERTAIN", + "duplicate_of": "icon4py-YYYY-MM-DD-XXXXXXX", + "reasoning": "..." +} +``` + +`duplicate_of` is required when `verdict` is `DUPLICATE`; it may be omitted or `null` otherwise. + +In your reasoning, be specific: quote relevant code, explain the validity +assessment, and if you vote `DUPLICATE` or `UNCERTAIN`, explain why. diff --git a/scripts/icon4py-review/agents/icon4py-fixedness-checker.md b/scripts/icon4py-review/agents/icon4py-fixedness-checker.md new file mode 100644 index 0000000..6cd66c0 --- /dev/null +++ b/scripts/icon4py-review/agents/icon4py-fixedness-checker.md @@ -0,0 +1,35 @@ +--- +name: icon4py-fixedness-checker +description: Checks whether an existing open issue is fixed in the current icon4py checkout. +prompt_mode: replace +tools: read, grep, find, ls, write, edit +--- + +Your job is to decide whether an existing open issue is resolved in the current +icon4py checkout. + +Your task prompt gives you `icon4py_checkout` (an absolute path to the clone), +the existing `issue` (id, title, description, file, lines, symbol, +suggested_fix), and `output_path` (where to write your verdict JSON). + +Read `/` around `` and the symbol +named ``. Check whether the code described by the issue still +exists, has been removed, or has been changed in a way that resolves it. + +Vote exactly one of: + +- `fixed`: the code that caused the issue is gone or changed so the issue no longer applies. +- `persists`: the issue is still present in the current code. +- `unknown`: you cannot decide (the code moved, the location is ambiguous, or you cannot reach a conclusion). + +In your reasoning, quote the current code and explain how it relates to the issue. + +Write a JSON file to `output_path`: + +```json +{ + "voter": "icon4py-fixedness-checker", + "verdict": "fixed", + "reasoning": "..." +} +``` diff --git a/scripts/icon4py-review/agents/icon4py-performance-reviewer.md b/scripts/icon4py-review/agents/icon4py-performance-reviewer.md new file mode 100644 index 0000000..f05ae49 --- /dev/null +++ b/scripts/icon4py-review/agents/icon4py-performance-reviewer.md @@ -0,0 +1,77 @@ +--- +name: icon4py-performance-reviewer +description: Review C2SM/icon4py for performance issues at the requested severity. +prompt_mode: replace +tools: read, grep, find, ls, write, edit +--- + +You are a specialist performance reviewer for the C2SM/icon4py climate/weather +model codebase. Find performance issues that can cause significant slowdown or +memory pressure on production GPU/MPI runs. + +Your task prompt gives you `icon4py_checkout` (an absolute path to the clone), +`requested_severity` (only look for and report issues with the given or more +severe severity), `output_path` (where to write the JSON findings file), and +`findings_dir` (writable directory shared with the orchestrator). + +Severity levels (assign one to every finding): +- `high`: significant performance degradation or memory pressure on production + GPU/MPI runs. +- `medium`: a real but localized or conditional performance problem, e.g. only + under certain configurations, grid sizes, or cold code paths. +- `low`: a minor, evidence-backed optimization opportunity with limited impact. + +Before reviewing, read the icon4py AGENTS.md at `/AGENTS.md` +and any referenced coding-guideline files for the codebase conventions. + +Start by reading `/open-issues.json` so you do not re-report +issues that are already tracked. You may still examine the same files for other +issues. + +Then read `/changes.diff` for the code that changed since the last +review. Prioritize the changed code, but also examine the rest of the checkout. +If `changes.diff` is absent, review the full checkout. + +Performance issues include, but are not limited to: + +- Unnecessary host/device memory copies inside hot paths. +- Redundant or overly broad halo exchanges. +- Temporary allocations inside timestep, substep, or iteration loops. +- Missing opportunities for GT4Py fusion. +- Safe but wasteful domain over-computation. +- Algorithmic complexity problems in paths that will scale. + +Do not report micro-optimizations or speculative changes without concrete evidence. + +Write a single valid JSON file to `output_path` with exactly this shape: + +```json +{ + "reviewer": "icon4py-performance-reviewer", + "findings": [ + { + "fingerprint": "performance:::", + "title": "...", + "severity": "high", + "description": "...", + "evidence": "...", + "file": "", + "lines": [start, end], + "symbol": "...", + "suggested_fix": "..." + } + ] +} +``` + +`description`, `evidence`, and `suggested_fix` are inline Markdown (no headings): +use backticks for code, symbols, and file paths; `*` for emphasis; lists where +useful. The issue file supplies its own `## Summary` / `## Evidence` / `## +Suggested fix` headings; do not include headings in these field values. + +`tags` is a list of 3-6 short kebab-case keywords that help someone scan the +tracker for related issues (e.g. `gpu`, `mpi`, `halo-exchange`, `memory`). Do +not include the reviewer name, the date, or any string longer than a short +phrase. + +If there are no findings at the requested severity, write `{"reviewer": "icon4py-performance-reviewer", "findings": []}`. diff --git a/scripts/icon4py-review/collect-open-issues.py b/scripts/icon4py-review/collect-open-issues.py new file mode 100755 index 0000000..1286121 --- /dev/null +++ b/scripts/icon4py-review/collect-open-issues.py @@ -0,0 +1,93 @@ +#!/usr/bin/env -S uv run --script +# /// script +# requires-python = ">=3.11" +# dependencies = ["pyyaml"] +# /// +"""Collect existing open issues into a small JSON list for the orchestrator. + +The orchestrator runs inside a sandbox with no access to content/review/issues/. +This pre-step reads those files and writes one entry per open issue (id, +fingerprint, source file, last-seen commit, lines, symbol) to a findings dir. +The orchestrator then decides, purely from this JSON, which open issues to send +to the fixedness panel. +""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path + +import yaml + + +def split_frontmatter(text: str) -> tuple[dict, str] | None: + if not text.startswith("---\n"): + return None + rest = text[4:] + parts = rest.split("\n---\n", 1) + if len(parts) < 2: + return None + try: + frontmatter = yaml.safe_load(parts[0]) or {} + except yaml.YAMLError: + return None + return frontmatter, parts[1] + + +def section(body: str, heading: str) -> str: + # Extract the text under a `## heading` until the next `## ` or end. + marker = f"## {heading}\n" + start = body.find(marker) + if start == -1: + return "" + start += len(marker) + nxt = body.find("\n## ", start) + return body[start:nxt].strip() if nxt != -1 else body[start:].strip() + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description="Collect existing open issues for the orchestrator.") + parser.add_argument("issues_dir", type=Path, help="Path to content/review/issues/") + parser.add_argument("output", type=Path, help="Path to write open-issues.json") + args = parser.parse_args(argv) + + if not args.issues_dir.is_dir(): + print(f"Error: not a directory: {args.issues_dir}", file=sys.stderr) + return 1 + + entries: list[dict] = [] + for path in sorted(args.issues_dir.glob("*.md")): + if path.name == ".gitkeep": + continue + result = split_frontmatter(path.read_text(encoding="utf-8")) + if result is None: + print(f"Warning: skipping {path}, malformed frontmatter", file=sys.stderr) + continue + fm, body = result + if fm.get("issue_status", "open") != "open": + continue + source = fm.get("source") or {} + entries.append( + { + "id": fm.get("id", path.stem), + "fingerprint": fm.get("fingerprint", ""), + "file": source.get("file", ""), + "commit_sha": source.get("commit_sha", ""), + "lines": source.get("lines", [0, 0]), + "symbol": source.get("symbol", ""), + "title": fm.get("title", ""), + "description": section(body, "Summary"), + "suggested_fix": section(body, "Suggested fix"), + } + ) + + args.output.parent.mkdir(parents=True, exist_ok=True) + args.output.write_text(json.dumps(entries, indent=2) + "\n", encoding="utf-8") + print(f"Collected {len(entries)} open issue(s) to {args.output}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/icon4py-review/commit-and-pr.sh b/scripts/icon4py-review/commit-and-pr.sh new file mode 100755 index 0000000..beef821 --- /dev/null +++ b/scripts/icon4py-review/commit-and-pr.sh @@ -0,0 +1,57 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Commit review changes to a dated branch and open a pull request. +# The PR description is the overview report itself. +# +# Usage: commit-and-pr.sh +# Intended to run from GitHub Actions only. + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +cd "$REPO_ROOT" + +report="${1:?usage: commit-and-pr.sh }" +if [[ ! -f "$report" ]]; then + echo "Error: report not found: $report" >&2 + exit 1 +fi + +if [[ -z "${GITHUB_TOKEN:-}" ]]; then + echo "Error: GITHUB_TOKEN is not set." >&2 + exit 1 +fi + +if ! git diff --cached --quiet 2>/dev/null; then + echo "Error: there are already staged changes. This script stages content/review/ itself." >&2 + exit 1 +fi + +# Use -c to avoid modifying the user's local git config. +git -c user.name="icon4py-review-bot" \ + -c user.email="icon4py-review-bot@users.noreply.github.com" \ + add content/review/ + +if git -c user.name="icon4py-review-bot" -c user.email="icon4py-review-bot@users.noreply.github.com" diff --cached --quiet; then + echo "No review changes to commit." + exit 0 +fi + +week=$(date +%G-W%V) +branch="review/week-${week}-$(date +%s%N)" +git -c user.name="icon4py-review-bot" \ + -c user.email="icon4py-review-bot@users.noreply.github.com" \ + commit -m "review(week ${week}): update icon4py findings" + +if ! git push origin "$branch"; then + echo "Error: failed to push branch $branch" >&2 + exit 1 +fi + +if ! gh pr create \ + --title "icon4py week ${week} review" \ + --body-file "$report" \ + --base main \ + --head "$branch"; then + echo "Error: failed to create pull request" >&2 + exit 1 +fi diff --git a/scripts/icon4py-review/extract-report-commit.py b/scripts/icon4py-review/extract-report-commit.py new file mode 100755 index 0000000..caec8c6 --- /dev/null +++ b/scripts/icon4py-review/extract-report-commit.py @@ -0,0 +1,59 @@ +#!/usr/bin/env -S uv run --script +# /// script +# requires-python = ">=3.11" +# dependencies = ["pyyaml"] +# /// +"""Extract the icon4py commit recorded in a review report's frontmatter. + +`run.sh` calls this on the newest report in content/review/reports/ to find the +commit the previous review ran against. That commit is the diff baseline for +the next run's `changes.diff`. Prints nothing (and exits 0) if the report has +no `icon4py_commit` frontmatter field, so older reports fall back gracefully. +""" + +from __future__ import annotations + +import argparse +import sys +from pathlib import Path + +import yaml + + +def split_frontmatter(text: str) -> tuple[dict, str] | None: + if not text.startswith("---\n"): + return None + rest = text[4:] + parts = rest.split("\n---\n", 1) + if len(parts) < 2: + return None + try: + frontmatter = yaml.safe_load(parts[0]) or {} + except yaml.YAMLError: + return None + return frontmatter, parts[1] + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description="Extract the icon4py_commit from a review report's frontmatter." + ) + parser.add_argument("report", type=Path, help="Path to a review report .md file") + args = parser.parse_args(argv) + + if not args.report.is_file(): + print(f"Error: not a file: {args.report}", file=sys.stderr) + return 1 + + result = split_frontmatter(args.report.read_text(encoding="utf-8")) + if result is None: + return 0 + frontmatter, _ = result + commit = frontmatter.get("icon4py_commit") + if commit: + print(commit) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/icon4py-review/models.json b/scripts/icon4py-review/models.json new file mode 100644 index 0000000..0883b57 --- /dev/null +++ b/scripts/icon4py-review/models.json @@ -0,0 +1,18 @@ +{ + "providers": { + "cscs-inference": { + "name": "CSCS Inference", + "api": "openai-completions", + "baseUrl": "https://api.inference.cscs.ch/v1", + "apiKey": "$CSCS_INFERENCE_API_KEY", + "models": [ + { + "id": "zai-org/GLM-5.2", + "name": "GLM 5.2", + "contextWindow": 1048576, + "maxTokens": 131072 + } + ] + } + } +} diff --git a/scripts/icon4py-review/reconcile-issues.py b/scripts/icon4py-review/reconcile-issues.py new file mode 100755 index 0000000..4558f37 --- /dev/null +++ b/scripts/icon4py-review/reconcile-issues.py @@ -0,0 +1,471 @@ +#!/usr/bin/env -S uv run --script +# /// script +# requires-python = ">=3.11" +# dependencies = ["pyyaml"] +# /// +"""Apply orchestrator output to the Markdown issue files. + +Reads accepted findings and fixedness verdicts (both JSON written by the +orchestrator), plus the existing issue files, and creates, updates, or marks +issues. The orchestrator makes every LLM decision; this script only does the +deterministic bookkeeping (fingerprint matching, stable ids, frontmatter +assembly, human-field preservation, history) that the LLM does unreliably. + +Input contract: +- accepted.json: a flat list of accepted findings (fingerprint matched this run). +- fixedness.json: a flat list, one entry per existing open issue that was NOT + matched this run. Each entry has fingerprint and verdict in + {fixed, persists, unknown, not-detected}. "not-detected" means the reviewers + saw the same commit and missed it; "unknown" means the panel could not decide + (possibly stale). Only "fixed" marks an issue fixed. + +Human authority: +- issue_status: invalid is preserved and never reopened. +- human_note is preserved across body regeneration. +- A human-marked fixed issue is reopened only if the same fingerprint is + detected again; a fixedness verdict alone never reopens a human dismissal. +""" + +from __future__ import annotations + +import argparse +import hashlib +import json +import re +import sys +from dataclasses import dataclass +from pathlib import Path + +import yaml + +ID_RE = re.compile(r"^icon4py-\d{4}-\d{2}-\d{2}-[a-z0-9]{7}$") +STATUS_VALUES = {"open", "fixed", "invalid"} +VERDICT_VALUES = {"fixed", "persists", "unknown", "not-detected"} +REQUIRED_FINDING_FIELDS = { + "title", + "severity", + "confidence", + "description", + "evidence", + "file", + "lines", + "symbol", + "suggested_fix", + "reviewer", + "fingerprint", +} + + +@dataclass(frozen=True) +class Source: + repo: str + ref: str + commit_sha: str + file: str + lines: list[int] + symbol: str + + def as_dict(self) -> dict: + return { + "repo": self.repo, + "ref": self.ref, + "commit_sha": self.commit_sha, + "file": self.file, + "lines": self.lines, + "symbol": self.symbol, + } + + +@dataclass +class Issue: + path: Path + frontmatter: dict + body: str + + @property + def id(self) -> str | None: + return self.frontmatter.get("id") + + @property + def fingerprint(self) -> str | None: + return self.frontmatter.get("fingerprint") + + @property + def status(self) -> str: + return self.frontmatter.get("issue_status", "open") + + def write(self) -> None: + text = "---\n" + yaml.safe_dump(self.frontmatter, sort_keys=False, allow_unicode=True) + "---\n" + self.body + self.path.write_text(text, encoding="utf-8") + + +def split_frontmatter(text: str) -> tuple[dict, str] | None: + if not text.startswith("---\n"): + return None + rest = text[4:] + parts = rest.split("\n---\n", 1) + if len(parts) < 2: + return None + try: + frontmatter = yaml.safe_load(parts[0]) or {} + except yaml.YAMLError: + return None + return frontmatter, parts[1] + + +def load_issues(issues_dir: Path) -> dict[str, Issue]: + issues: dict[str, Issue] = {} + for path in issues_dir.glob("*.md"): + if path.name == ".gitkeep": + continue + text = path.read_text(encoding="utf-8") + result = split_frontmatter(text) + if result is None: + continue + frontmatter, body = result + fp = frontmatter.get("fingerprint") + if fp: + issues[fp] = Issue(path=path, frontmatter=frontmatter, body=body) + return issues + + +def issue_id(date: str, fingerprint: str) -> str: + suffix = hashlib.sha256(fingerprint.encode()).hexdigest()[:7] + return f"icon4py-{date}-{suffix}" + + +def issue_body(finding: dict) -> str: + sections = [ + "## Summary\n\n" + finding.get("description", ""), + ] + impact = finding.get("impact") + if impact: + sections.append("## Impact\n\n" + impact) + sections.append("## Evidence\n\n" + finding.get("evidence", "")) + sections.append("## Suggested fix\n\n" + finding.get("suggested_fix", "")) + human_note = finding.get("human_note") + if human_note: + sections.append("## Human note\n\n" + human_note) + return "\n\n".join(sections) + "\n" + + +def update_issue(issue: Issue, finding: dict, date: str, run_id: str, commit_sha: str) -> None: + fm = issue.frontmatter + new_source = Source( + repo="C2SM/icon4py", + ref="main", + commit_sha=commit_sha, + file=finding["file"], + lines=list(finding.get("lines", [0, 0])), + symbol=finding.get("symbol", ""), + ) + + # Preserve human-owned fields. + human_note = fm.get("human_note") + previous_status = fm.get("issue_status", "open") + + fm["title"] = finding["title"] + fm["severity"] = finding["severity"] + fm["confidence"] = finding["confidence"] + existing_tags = set(fm.get("tags", [])) + if "tags" in finding and isinstance(finding["tags"], list): + existing_tags.update(finding["tags"]) + fm["tags"] = sorted(existing_tags) + fm["updated"] = date + fm["last_seen"] = date + fm["source"] = new_source.as_dict() + fm["found_by"] = list(dict.fromkeys(fm.get("found_by", []) + [finding["reviewer"]])) + fm["run_id"] = run_id + + history = fm.setdefault("history", []) + + # Reopen a human-marked fixed issue if the same fingerprint is detected + # again. A human-marked invalid issue is never reopened. + if previous_status == "fixed": + fm["issue_status"] = "open" + history.append( + { + "date": date, + "event": "reopened", + "run_id": run_id, + "commit_sha": commit_sha, + "reason": "same fingerprint detected again", + } + ) + + history.append( + { + "date": date, + "event": "sighting", + "run_id": run_id, + "commit_sha": commit_sha, + "note": "fingerprint matched this run", + } + ) + + # Regenerate the body from the current finding, preserving a human note. + body_finding = dict(finding) + if human_note: + body_finding["human_note"] = human_note + issue.body = issue_body(body_finding) + issue.write() + + +def create_issue( + issues_dir: Path, + finding: dict, + date: str, + run_id: str, + commit_sha: str, +) -> Issue: + fp = finding["fingerprint"] + new_id = issue_id(date, fp) + path = issues_dir / f"{new_id}.md" + + source = Source( + repo="C2SM/icon4py", + ref="main", + commit_sha=commit_sha, + file=finding["file"], + lines=list(finding.get("lines", [0, 0])), + symbol=finding.get("symbol", ""), + ) + + tags = finding.get("tags") or [] + fm = { + "id": new_id, + "title": finding["title"], + "issue_status": "open", + "severity": finding["severity"], + "confidence": finding["confidence"], + "fingerprint": fp, + "tags": sorted(set(tags)) if isinstance(tags, list) else [], + "created": date, + "updated": date, + "last_seen": date, + "source": source.as_dict(), + "found_by": [finding["reviewer"]], + "run_id": run_id, + "history": [ + { + "date": date, + "event": "detected", + "run_id": run_id, + "commit_sha": commit_sha, + } + ], + } + + issue = Issue(path=path, frontmatter=fm, body=issue_body(finding)) + issue.write() + return issue + + +def apply_fixedness(issue: Issue, verdict: str, date: str, run_id: str, commit_sha: str) -> str: + """Return the resulting issue_status after applying a fixedness verdict.""" + fm = issue.frontmatter + fm["updated"] = date + fm["run_id"] = run_id + history = fm.setdefault("history", []) + + note = { + "fixed": "fixedness panel confirmed the issue is resolved", + "persists": "fixedness panel confirmed the issue still exists", + "unknown": "fixedness panel could not decide; possibly stale", + "not-detected": "reviewers saw the same commit and did not reproduce", + }.get(verdict, verdict) + + if verdict == "fixed": + fm["issue_status"] = "fixed" + history.append({"date": date, "event": "fixed", "run_id": run_id, "commit_sha": commit_sha, "note": note}) + else: + # persist / unknown / not-detected all leave the issue open. + history.append({"date": date, "event": verdict, "run_id": run_id, "commit_sha": commit_sha, "note": note}) + + issue.write() + return fm["issue_status"] + + +def load_json_list(path: Path, label: str) -> list: + try: + with open(path, encoding="utf-8") as f: + data = json.load(f) + except (json.JSONDecodeError, OSError) as exc: + print(f"Error: failed to parse {label} ({path}): {exc}", file=sys.stderr) + raise SystemExit(1) + if not isinstance(data, list): + print(f"Error: {label} ({path}) must be a JSON list, not {type(data).__name__}", file=sys.stderr) + raise SystemExit(1) + return data + + +def load_duplicates(path: Path | None) -> list[dict]: + """Load duplicates.json if provided; return an empty list otherwise.""" + if path is None or not path.exists(): + return [] + try: + with open(path, encoding="utf-8") as f: + data = json.load(f) + except (json.JSONDecodeError, OSError) as exc: + print(f"Error: failed to parse duplicates ({path}): {exc}", file=sys.stderr) + raise SystemExit(1) + if not isinstance(data, list): + print(f"Error: duplicates ({path}) must be a JSON list", file=sys.stderr) + raise SystemExit(1) + return data + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description="Apply accepted findings and fixedness verdicts to issue files.") + parser.add_argument("accepted", type=Path, help="Path to accepted.json") + parser.add_argument("fixedness", type=Path, help="Path to fixedness.json") + parser.add_argument("issues_dir", type=Path, help="Path to content/review/issues/") + parser.add_argument("--date", required=True, help="Review date YYYY-MM-DD") + parser.add_argument("--run-id", required=True, help="Run identifier") + parser.add_argument("--commit-sha", required=True, help="icon4py commit SHA") + parser.add_argument("--duplicates", type=Path, help="Path to duplicates.json") + args = parser.parse_args(argv) + + for p in (args.accepted, args.fixedness): + if not p.exists(): + print(f"Error: not found: {p}", file=sys.stderr) + return 1 + + args.issues_dir.mkdir(parents=True, exist_ok=True) + + accepted = load_json_list(args.accepted, "accepted.json") + fixedness = load_json_list(args.fixedness, "fixedness.json") + duplicate_entries = load_duplicates(args.duplicates) + + existing = load_issues(args.issues_dir) + + # Build lookup of existing issues by id, used for duplicate merges. + existing_by_id: dict[str, Issue] = {} + for issue in existing.values(): + issue_id = issue.id + if issue_id: + existing_by_id[issue_id] = issue + + created = 0 + updated = 0 + merged_duplicates = 0 + marked_fixed = 0 + other = 0 + + # 1. Apply duplicate merges: update existing issues with a history note. + for entry in duplicate_entries: + if not isinstance(entry, dict): + continue + finding = entry.get("finding") or {} + target_id = entry.get("duplicate_of") + target_fp = entry.get("duplicate_of_fingerprint") + issue = existing_by_id.get(target_id) if target_id else None + if issue is None and target_fp: + issue = existing.get(target_fp) + if issue is None: + print( + f"Warning: duplicate entry targets missing issue {target_id!r} ({target_fp!r}); skipping", + file=sys.stderr, + ) + continue + + fm = issue.frontmatter + fm["updated"] = args.date + fm["last_seen"] = args.date + fm["run_id"] = args.run_id + # Merge tags from the new finding without rewriting the body. + if "tags" in finding and isinstance(finding["tags"], list): + existing_tags = set(fm.get("tags", [])) + existing_tags.update(finding["tags"]) + fm["tags"] = sorted(existing_tags) + + history = fm.setdefault("history", []) + history.append( + { + "date": args.date, + "event": "re-detected", + "run_id": args.run_id, + "commit_sha": args.commit_sha, + "note": f"weekly review detected the same issue again as {finding.get('fingerprint', '')}", + } + ) + issue.write() + merged_duplicates += 1 + # Do not apply fixedness to this existing issue. + existing.pop(issue.fingerprint, None) + + # 2. Apply accepted findings. + for finding in accepted: + if not isinstance(finding, dict): + print("Warning: skipping non-dict accepted entry", file=sys.stderr) + continue + missing = REQUIRED_FINDING_FIELDS - set(finding.keys()) + if missing: + fp = finding.get("fingerprint", "") + print(f"Warning: skipping accepted entry {fp!r} missing fields: {sorted(missing)}", file=sys.stderr) + continue + fp = finding["fingerprint"] + issue = existing.get(fp) + if issue is None: + create_issue(args.issues_dir, finding, args.date, args.run_id, args.commit_sha) + created += 1 + else: + update_issue(issue, finding, args.date, args.run_id, args.commit_sha) + updated += 1 + # An accepted finding re-confirms an open issue; do not also apply + # a fixedness verdict to the same fingerprint below. + existing.pop(fp, None) + + # 3. Apply fixedness verdicts to the remaining (unmatched) open issues. + # Defensive merge: if the orchestrator writes one entry per checker vote + # instead of one entry per issue, aggregate the votes here. + votes_by_fp: dict[str, list[str]] = {} + for entry in fixedness: + if not isinstance(entry, dict): + continue + fp = entry.get("fingerprint") + verdict = entry.get("verdict") + if not fp or verdict not in VERDICT_VALUES: + print(f"Warning: skipping fixedness entry {fp!r} verdict {verdict!r}", file=sys.stderr) + continue + votes_by_fp.setdefault(fp, []).append(verdict) + + def merge_fixedness_votes(votes: list[str]) -> str: + counts = {v: votes.count(v) for v in VERDICT_VALUES} + if counts.get("fixed", 0) >= 2: + return "fixed" + if counts.get("persists", 0) >= 2: + return "persists" + return "unknown" + + verdict_by_fp: dict[str, str] = {} + for fp, votes in votes_by_fp.items(): + if len(votes) > 1: + print(f"Note: merging {len(votes)} fixedness entries for {fp!r}", file=sys.stderr) + verdict_by_fp[fp] = merge_fixedness_votes(votes) + + for fp, issue in existing.items(): + if issue.status != "open": + continue + verdict = verdict_by_fp.get(fp) + if verdict is None: + # Not in accepted and not in fixedness: the orchestrator should not + # leave this case, but if it does, do not silently mark fixed. + print(f"Warning: open issue {issue.id!r} ({fp!r}) has no accepted or fixedness entry", file=sys.stderr) + continue + result = apply_fixedness(issue, verdict, args.date, args.run_id, args.commit_sha) + if result == "fixed": + marked_fixed += 1 + else: + other += 1 + + print( + f"Reconciled: {created} created, {updated} updated, " + f"{merged_duplicates} duplicates merged, " + f"{marked_fixed} marked fixed, {other} left open (persists/unknown/not-detected)." + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/icon4py-review/run.sh b/scripts/icon4py-review/run.sh new file mode 100755 index 0000000..ff4ca05 --- /dev/null +++ b/scripts/icon4py-review/run.sh @@ -0,0 +1,145 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Run the icon4py weekly review end to end. This is the single entry point for +# both local testing and CI. In CI, pass --commit-and-pr to commit and open a +# pull request; locally it just produces changes for inspection. +# +# Requires: +# - pi and bwrap installed and on PATH +# - ICON4PY_CHECKOUT pointing to a clone of C2SM/icon4py +# +# Auth: pi authenticates from auth.json in the review config dir (local +# testing) or CSCS_INFERENCE_API_KEY (CI, forwarded into the sandbox). run.sh +# does not check auth; pi fails loudly if neither is available. + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +PI_CONFIG_DIR="$REPO_ROOT/scripts/icon4py-review" +COMMIT_AND_PR=0 +for arg in "$@"; do + case "$arg" in + --commit-and-pr) COMMIT_AND_PR=1 ;; + esac +done + +if [[ -z "${ICON4PY_CHECKOUT:-}" ]]; then + echo "Error: ICON4PY_CHECKOUT is not set." >&2 + exit 1 +fi + +if [[ ! -d "$ICON4PY_CHECKOUT/.git" ]]; then + echo "Error: ICON4PY_CHECKOUT does not point to a git clone: $ICON4PY_CHECKOUT" >&2 + exit 1 +fi + +command -v pi >/dev/null || { echo "Error: pi not found on PATH." >&2; exit 1; } +command -v bwrap >/dev/null || { echo "Error: bwrap not found on PATH." >&2; exit 1; } +pi --version +bwrap --version | head -1 + +ICON4PY_SHA="$(git -C "$ICON4PY_CHECKOUT" rev-parse HEAD)" +TODAY="$(date +%Y-%m-%d)" +RUN_ID="weekly-$(date +%G-W%V)" +echo "icon4py commit: $ICON4PY_SHA" +echo "review date: $TODAY run id: $RUN_ID" + +REPORTS_DIR="$REPO_ROOT/content/review/reports" +ISSUES_DIR="$REPO_ROOT/content/review/issues" +FINDINGS_DIR="$(mktemp -d)" +SANDBOX_REPORTS_DIR="$(mktemp -d)" +trap 'rm -rf "$FINDINGS_DIR" "$SANDBOX_REPORTS_DIR"' EXIT + +mkdir -p "$REPORTS_DIR" "$ISSUES_DIR" + +# Pre-step: collect existing open issues into JSON the orchestrator can read. +echo "Collecting existing open issues..." +"$REPO_ROOT/scripts/icon4py-review/collect-open-issues.py" "$ISSUES_DIR" "$FINDINGS_DIR/open-issues.json" + +# Pre-step: write the full diff since the previous review, if a baseline exists. +# Reviewers cannot run git themselves (no bash, .git hidden in the sandbox), so +# the host generates the diff. On the first run, or if the previous report has +# no icon4py_commit frontmatter, skip the diff and let reviewers review as they +# see fit. +echo "Generating diff since previous review..." +# Find the newest report by mtime. nullglob makes an empty reports/ expand to +# nothing instead of the literal glob, which would fail `ls` under `set -euo +# pipefail`. +shopt -s nullglob +reports=("$REPORTS_DIR"/*.md) +shopt -u nullglob +PREV_COMMIT="" +if (( ${#reports[@]} > 0 )); then + latest_report="$(ls -t "${reports[@]}" | head -1)" + PREV_COMMIT="$("$REPO_ROOT/scripts/icon4py-review/extract-report-commit.py" "$latest_report" 2>/dev/null || true)" +fi +if [[ -n "$PREV_COMMIT" ]]; then + # The diff is a best-effort prioritization hint, not a gate. If the + # baseline commit is not present in the local checkout (shallow clone, + # pruned history), fall back to an empty changes.diff and let reviewers + # review as they see fit rather than aborting the whole run under set -e. + if git -C "$ICON4PY_CHECKOUT" diff "$PREV_COMMIT..$ICON4PY_SHA" > "$FINDINGS_DIR/changes.diff" 2>/dev/null; then + echo "Wrote changes.diff ($PREV_COMMIT..$ICON4PY_SHA), $(wc -l < "$FINDINGS_DIR/changes.diff") lines." + else + : > "$FINDINGS_DIR/changes.diff" + echo "Warning: git diff $PREV_COMMIT..$ICON4PY_SHA failed (commit not local?); wrote empty changes.diff." >&2 + fi +else + echo "No previous review commit found; skipping changes.diff." +fi + +# Run the orchestrator in the sandbox. It has no access to the issues +# directory; it reads open-issues.json and writes accepted.json, +# fixedness.json, and the overview report. +echo "Running isolated pi review..." +PI_SANDBOX_CHDIR="/tmp" \ +PI_SANDBOX_REPORTS_DIR="$SANDBOX_REPORTS_DIR" \ +PI_SANDBOX_FINDINGS_DIR="$FINDINGS_DIR" \ +PI_SANDBOX_TOOLS="subagent,get_subagent_result,read,ls,write,edit" \ +ICON4PY_CHECKOUT="$ICON4PY_CHECKOUT" \ + "$REPO_ROOT/scripts/pi-sandboxed.sh" \ + "$PI_CONFIG_DIR" \ + "$PI_CONFIG_DIR" \ + "Run the weekly icon4py review. icon4py_checkout=$ICON4PY_CHECKOUT. icon4py_commit=$ICON4PY_SHA. review_date=$TODAY. run_id=$RUN_ID. requested_severity=high. findings_dir=/tmp/icon4py-review-findings. reports_dir=/tmp/review-reports. See the skill for the workflow." + +# The orchestrator writes the report inside the sandbox. Pull it into the repo. +# The filename includes hour-minute (e.g. 2026-08-05-2114.md) so same-day runs +# do not conflict. Glob for the newest one. +report="$(ls -t "$SANDBOX_REPORTS_DIR"/*.md 2>/dev/null | head -1)" +if [[ -z "$report" || ! -s "$report" ]]; then + echo "Error: missing or empty overview report in $SANDBOX_REPORTS_DIR" >&2 + exit 1 +fi +mv "$report" "$REPORTS_DIR/$(basename "$report")" + +if [[ -f "$FINDINGS_DIR/accepted.json" && -f "$FINDINGS_DIR/fixedness.json" ]]; then + echo "Reconciling accepted findings and fixedness verdicts..." + duplicates_arg="" + if [[ -f "$FINDINGS_DIR/duplicates.json" ]]; then + duplicates_arg="--duplicates $FINDINGS_DIR/duplicates.json" + fi + "$REPO_ROOT/scripts/icon4py-review/reconcile-issues.py" \ + "$FINDINGS_DIR/accepted.json" \ + "$FINDINGS_DIR/fixedness.json" \ + "$ISSUES_DIR" \ + --date "$TODAY" \ + --run-id "$RUN_ID" \ + --commit-sha "$ICON4PY_SHA" \ + $duplicates_arg +else + echo "Warning: accepted.json or fixedness.json missing; skipping reconciliation." >&2 +fi + +echo "Validating issues..." +"$REPO_ROOT/scripts/icon4py-review/validate-issues.py" "$ISSUES_DIR" +echo "Regenerating index..." +"$REPO_ROOT/scripts/icon4py-review/update-index.py" "$ISSUES_DIR" "$REPO_ROOT/content/review/index.md" + +# The newest report (glob for the hour-minute file we moved in). +report="$(ls -t "$REPORTS_DIR"/*.md 2>/dev/null | head -1)" + +if [[ "$COMMIT_AND_PR" -eq 1 ]]; then + echo "Committing and opening pull request..." + "$REPO_ROOT/scripts/icon4py-review/commit-and-pr.sh" "$report" +else + echo "Done. Inspect changes with: git status --short content/review/" +fi diff --git a/scripts/icon4py-review/settings.json b/scripts/icon4py-review/settings.json new file mode 100644 index 0000000..0801bb4 --- /dev/null +++ b/scripts/icon4py-review/settings.json @@ -0,0 +1,7 @@ +{ + "defaultProvider": "cscs-inference", + "defaultModel": "zai-org/GLM-5.2", + "packages": [ + "npm:@gotgenes/pi-subagents@19.2.1" + ] +} diff --git a/scripts/icon4py-review/update-index.py b/scripts/icon4py-review/update-index.py new file mode 100755 index 0000000..f27ae83 --- /dev/null +++ b/scripts/icon4py-review/update-index.py @@ -0,0 +1,123 @@ +#!/usr/bin/env -S uv run --script +# /// script +# requires-python = ">=3.11" +# dependencies = ["pyyaml"] +# /// +"""Regenerate content/review/index.md from the current issue files.""" + +from __future__ import annotations + +import argparse +import sys +from pathlib import Path + +import yaml + + +def split_frontmatter(text: str) -> dict | None: + if not text.startswith("---\n"): + return None + rest = text[4:] + parts = rest.split("\n---\n", 1) + if len(parts) < 2: + return None + try: + return yaml.safe_load(parts[0]) or {} + except yaml.YAMLError: + return None + + +def read_issue(path: Path) -> dict | None: + return split_frontmatter(path.read_text(encoding="utf-8")) + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description="Regenerate review index.") + parser.add_argument("issues_dir", type=Path, help="Path to content/review/issues/") + parser.add_argument("index_path", type=Path, help="Path to content/review/index.md") + args = parser.parse_args(argv) + + if not args.issues_dir.is_dir(): + print(f"Error: not a directory: {args.issues_dir}", file=sys.stderr) + return 1 + + issues: dict[str, list[dict]] = {"open": [], "fixed": [], "invalid": []} + for path in sorted(args.issues_dir.glob("*.md")): + if path.name == ".gitkeep": + continue + frontmatter = read_issue(path) + if frontmatter is None: + continue + status = frontmatter.get("issue_status", "open") + issues.setdefault(status, []).append( + { + "id": frontmatter.get("id", path.stem), + "title": frontmatter.get("title", "Untitled"), + "severity": frontmatter.get("severity", "unknown"), + "tags": frontmatter.get("tags", []), + "path": f"review/issues/{path.name}", + } + ) + + # Reports are named YYYY-MM-DD-HHMM.md, so reverse-lexicographic + # filename order is newest-first. A no-HHMM filename would sort after + # a same-day HHMM one and break this; the report writer must use HHMM. + reports_dir = args.issues_dir.parent / "reports" + reports: list[dict] = [] + if reports_dir.is_dir(): + for path in sorted(reports_dir.glob("*.md"), reverse=True): + if path.name == ".gitkeep": + continue + fm = read_issue(path) + reports.append({ + "title": fm.get("title", "Untitled") if fm else "Untitled", + "created": fm.get("created", "") if fm else "", + "path": f"review/reports/{path.name}", + }) + + lines = [ + "---", + "title: Automated icon4py review tracker", + "---", + "", + "", + "", + "This page tracks automated review findings for C2SM/icon4py. " + "Individual issue files live in `review/issues/`. " + "Weekly overview reports live in `review/reports/`.", + "", + ] + + for status in ("open", "fixed", "invalid"): + lines.append(f"## {status.capitalize()} issues") + lines.append("") + if not issues[status]: + lines.append(f"_No {status} issues._") + lines.append("") + continue + for issue in issues[status]: + tags = ", ".join(issue["tags"]) if issue["tags"] else "none" + lines.append( + f"- [[{issue['path']}|{issue['title']}]] " + f"- severity: {issue['severity']}, tags: {tags}" + ) + lines.append("") + + lines.append("## Reports") + lines.append("") + if not reports: + lines.append("_No reports._") + else: + for r in reports: + lines.append(f"- [[{r['path']}|{r['title']}]]") + lines.append("") + + args.index_path.parent.mkdir(parents=True, exist_ok=True) + args.index_path.write_text("\n".join(lines), encoding="utf-8") + total = sum(len(v) for v in issues.values()) + print(f"Wrote {args.index_path} with {total} issue(s) and {len(reports)} report(s).") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/icon4py-review/validate-issues.py b/scripts/icon4py-review/validate-issues.py new file mode 100755 index 0000000..0f6573f --- /dev/null +++ b/scripts/icon4py-review/validate-issues.py @@ -0,0 +1,154 @@ +#!/usr/bin/env -S uv run --script +# /// script +# requires-python = ">=3.11" +# dependencies = ["pyyaml"] +# /// +"""Validate review issue files under content/review/issues/.""" + +from __future__ import annotations + +import argparse +import hashlib +import re +import sys +from pathlib import Path + +import yaml + +REQUIRED_FIELDS = { + "id", + "title", + "issue_status", + "severity", + "confidence", + "fingerprint", + "tags", + "created", + "updated", + "last_seen", + "source", + "found_by", + "run_id", + "history", +} + +SOURCE_FIELDS = {"repo", "ref", "commit_sha", "file", "lines", "symbol"} +STATUS_VALUES = {"open", "fixed", "invalid"} +SEVERITY_VALUES = {"high", "medium", "low"} +CONFIDENCE_VALUES = {"high", "medium", "low"} +ID_RE = re.compile(r"^icon4py-\d{4}-\d{2}-\d{2}-[a-z0-9]{7}$") + + +def split_frontmatter(text: str) -> tuple[dict, str] | None: + if not text.startswith("---\n"): + return None + rest = text[4:] + parts = rest.split("\n---\n", 1) + if len(parts) < 2: + return None + try: + frontmatter = yaml.safe_load(parts[0]) or {} + except yaml.YAMLError: + return None + return frontmatter, parts[1] + + +def validate_issue(path: Path) -> list[str]: + errors: list[str] = [] + text = path.read_text(encoding="utf-8") + result = split_frontmatter(text) + if result is None: + errors.append(f"{path}: malformed frontmatter") + return errors + + frontmatter, _body = result + + missing = REQUIRED_FIELDS - set(frontmatter.keys()) + if missing: + errors.append(f"{path}: missing required fields: {sorted(missing)}") + + issue_id = frontmatter.get("id") + if issue_id and not ID_RE.match(issue_id): + errors.append(f"{path}: id {issue_id!r} does not match pattern icon4py-YYYY-MM-DD-XXXXXXX") + + expected_filename = f"{issue_id}.md" if issue_id else None + if expected_filename and path.name != expected_filename: + errors.append(f"{path}: filename {path.name!r} does not match id {expected_filename!r}") + + # Verify id suffix matches fingerprint hash. + fingerprint = frontmatter.get("fingerprint") + if fingerprint and issue_id: + expected_suffix = hashlib.sha256(fingerprint.encode()).hexdigest()[:7] + actual_suffix = issue_id.rsplit("-", 1)[-1] + if actual_suffix != expected_suffix: + errors.append( + f"{path}: id suffix {actual_suffix!r} does not match fingerprint hash {expected_suffix!r}" + ) + + if frontmatter.get("issue_status") not in STATUS_VALUES: + errors.append(f"{path}: issue_status must be one of {STATUS_VALUES}") + + if frontmatter.get("severity") not in SEVERITY_VALUES: + errors.append(f"{path}: severity must be one of {SEVERITY_VALUES}") + + if frontmatter.get("confidence") not in CONFIDENCE_VALUES: + errors.append(f"{path}: confidence must be one of {CONFIDENCE_VALUES}") + + source = frontmatter.get("source") or {} + missing_source = SOURCE_FIELDS - set(source.keys()) + if missing_source: + errors.append(f"{path}: source missing fields: {sorted(missing_source)}") + + lines = source.get("lines") + if lines is not None and (not isinstance(lines, list) or len(lines) != 2 or not all(isinstance(x, int) for x in lines)): + errors.append(f"{path}: source.lines must be a two-element integer list") + + history = frontmatter.get("history") + if history is not None and not isinstance(history, list): + errors.append(f"{path}: history must be a list") + + return errors + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description="Validate review issue files.") + parser.add_argument("issues_dir", type=Path, help="Path to content/review/issues/") + args = parser.parse_args(argv) + + if not args.issues_dir.is_dir(): + print(f"Error: not a directory: {args.issues_dir}", file=sys.stderr) + return 1 + + all_errors: list[str] = [] + fingerprints: dict[str, Path] = {} + count = 0 + for path in sorted(args.issues_dir.glob("*.md")): + if path.name == ".gitkeep": + continue + count += 1 + all_errors.extend(validate_issue(path)) + result = split_frontmatter(path.read_text(encoding="utf-8")) + if result is None: + continue + frontmatter, _ = result + fingerprint = frontmatter.get("fingerprint") + if fingerprint: + if fingerprint in fingerprints: + all_errors.append( + f"{path}: duplicate fingerprint {fingerprint!r} " + f"(also in {fingerprints[fingerprint]})" + ) + fingerprints[fingerprint] = path + + if all_errors: + print("Validation failed:", file=sys.stderr) + for error in all_errors: + print(f" - {error}", file=sys.stderr) + return 1 + + print(f"Validated {count} issue file(s). OK.") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/pi-sandboxed.sh b/scripts/pi-sandboxed.sh new file mode 100755 index 0000000..92bf2a5 --- /dev/null +++ b/scripts/pi-sandboxed.sh @@ -0,0 +1,147 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Run pi inside a bwrap sandbox with a dedicated config directory. +# +# Usage: +# CSCS_INFERENCE_API_KEY=... ./scripts/pi-sandboxed.sh +# +# Optional environment variables: +# PI_SANDBOX_CHDIR - working directory inside the sandbox (default: current cwd) +# PI_SANDBOX_REPORTS_DIR - path bound writable to /tmp/review-reports (overview report sink) +# PI_SANDBOX_FINDINGS_DIR - host path bound writable to /tmp/icon4py-review-findings +# PI_SANDBOX_EXTRA_BINDS - semicolon-separated src;dst pairs for additional binds +# PI_SANDBOX_ENABLE_GITHUB_TOKEN - if set, forward GITHUB_TOKEN into the sandbox +# PI_SANDBOX_TOOLS - comma-separated tool allowlist (default: read,write,bash) +# +# Isolation: +# - The whole host filesystem is mounted read-only. +# - /tmp is a fresh tmpfs. +# - Only PI_SANDBOX_REPORTS_DIR, PI_SANDBOX_FINDINGS_DIR, and extra binds are writable. +# - The pi config directory is copied to a fresh writable location inside +# the sandbox so the original cannot be modified. auth.json, if present, +# is a local auth source and is kept in the copy. +# - .git directories under PI_SANDBOX_CHDIR and ICON4PY_CHECKOUT are hidden. +# - Network is shared (--share-net): the only thing an agent without bash can +# reach is the inference provider. Adding bash to PI_SANDBOX_TOOLS reopens +# arbitrary network access and must be reconsidered. + +PI_CONFIG_DIR="${1:?pi config directory required}" +SKILL_PATH="${2:?skill path required}" +PROMPT="${3:?prompt required}" +PI_BIN="${PI_BIN:-$(command -v pi 2>/dev/null || true)}" +if [[ -z "$PI_BIN" ]]; then + echo "Error: pi binary not found. Install pi or set PI_BIN." >&2 + exit 1 +fi + +if [[ ! -d "$PI_CONFIG_DIR" ]]; then + echo "Error: pi config directory not found: $PI_CONFIG_DIR" >&2 + exit 1 +fi + +if [[ ! -f "$PI_CONFIG_DIR/settings.json" ]]; then + echo "Error: settings.json not found in $PI_CONFIG_DIR" >&2 + exit 1 +fi + +# Copy config dir to a writable temp location so the original cannot be +# modified. Auto-installed packages (e.g. @gotgenes/pi-subagents) are +# installed into this copy by pi at startup. auth.json, if present, is a +# local auth source and is intentionally kept. +TMP_CONFIG="$(mktemp -d /tmp/pi-config-XXXXXX)" +trap 'rm -rf "$TMP_CONFIG"' EXIT +cp -a "$PI_CONFIG_DIR"/. "$TMP_CONFIG/" +mkdir -p "$TMP_CONFIG/sessions" + +BWRAP_ARGS=( + bwrap + --die-with-parent + --clearenv + --ro-bind / / + --tmpfs /tmp + --bind "$TMP_CONFIG" /tmp/pi-config +) + +# Writable sink for the overview report (narrow: only reports/, never issues/). +if [[ -n "${PI_SANDBOX_REPORTS_DIR:-}" ]]; then + if [[ ! -d "$PI_SANDBOX_REPORTS_DIR" ]]; then + echo "Error: reports directory not found: $PI_SANDBOX_REPORTS_DIR" >&2 + exit 1 + fi + BWRAP_ARGS+=(--bind "$PI_SANDBOX_REPORTS_DIR" /tmp/review-reports) +fi + +# Writable sink for intermediate findings shared with the caller. +if [[ -n "${PI_SANDBOX_FINDINGS_DIR:-}" ]]; then + if [[ ! -d "$PI_SANDBOX_FINDINGS_DIR" ]]; then + echo "Error: findings directory not found: $PI_SANDBOX_FINDINGS_DIR" >&2 + exit 1 + fi + BWRAP_ARGS+=(--bind "$PI_SANDBOX_FINDINGS_DIR" /tmp/icon4py-review-findings) +fi + +# Optional extra binds; format is semicolon-separated src;dst pairs. +if [[ -n "${PI_SANDBOX_EXTRA_BINDS:-}" ]]; then + IFS=';' read -ra EXTRA_BIND_ARRAY <<< "$PI_SANDBOX_EXTRA_BINDS" + if (( ${#EXTRA_BIND_ARRAY[@]} % 2 != 0 )); then + echo "Error: PI_SANDBOX_EXTRA_BINDS must have an even number of semicolon-separated entries" >&2 + exit 1 + fi + for ((i = 0; i < ${#EXTRA_BIND_ARRAY[@]}; i += 2)); do + BWRAP_ARGS+=(--bind "${EXTRA_BIND_ARRAY[i]}" "${EXTRA_BIND_ARRAY[i+1]}") + done +fi + +# Bind the icon4py checkout read-only at its own path. --tmpfs /tmp above +# hides /tmp/ inside the sandbox, so without this the reviewers' +# icon4py_checkout points at a directory that does not exist and every read +# fails. Then hide .git under that checkout so no checkout credentials leak. +if [[ -n "${ICON4PY_CHECKOUT:-}" && -d "$ICON4PY_CHECKOUT" ]]; then + BWRAP_ARGS+=(--ro-bind "$ICON4PY_CHECKOUT" "$ICON4PY_CHECKOUT") + if [[ -d "$ICON4PY_CHECKOUT/.git" ]]; then + BWRAP_ARGS+=(--tmpfs "$ICON4PY_CHECKOUT/.git") + fi +fi + +BWRAP_ARGS+=( + --proc /proc + --dev /dev + --share-net + --setenv HOME /tmp + --setenv PI_CODING_AGENT_DIR /tmp/pi-config + # Forward the host PATH so the setup-node npm and node are reachable inside + # the sandbox (needed for pi's package auto-install and its wrapper). All of + # / is read-only, so no binary becomes newly writable. + --setenv PATH "$PATH" + --setenv TZ "${TZ:-Europe/Zurich}" + --setenv CSCS_INFERENCE_API_KEY "${CSCS_INFERENCE_API_KEY:-}" + --setenv ICON4PY_CHECKOUT "${ICON4PY_CHECKOUT:-}" + --setenv PI_SANDBOX_REPORTS_DIR "${PI_SANDBOX_REPORTS_DIR:+/tmp/review-reports}" + --setenv PI_SANDBOX_FINDINGS_DIR "${PI_SANDBOX_FINDINGS_DIR:+/tmp/icon4py-review-findings}" +) + +if [[ -n "${PI_SANDBOX_ENABLE_GITHUB_TOKEN:-}" ]]; then + BWRAP_ARGS+=(--setenv GITHUB_TOKEN "${GITHUB_TOKEN:-}") +fi + +if [[ -n "${PI_SANDBOX_CHDIR:-}" ]]; then + BWRAP_ARGS+=(--chdir "$PI_SANDBOX_CHDIR") +fi + +PI_SANDBOX_TOOLS="${PI_SANDBOX_TOOLS:-read,write,bash}" + +# Invoke pi through bwrap with the assembled args. Earlier this exec'd pi +# directly, bypassing the sandbox entirely (the env leaked, global extensions +# like AFT/memory loaded, and pi never exited). The BWRAP_ARGS array +# (--clearenv, --ro-bind / /, --setenv PI_CODING_AGENT_DIR /tmp/pi-config, +# --setenv HOME /tmp, etc.) only takes effect when bwrap is the entry point. +BWRAP_ARGS+=( + "$PI_BIN" + -p --approve + --tools "$PI_SANDBOX_TOOLS" + --skill "$SKILL_PATH" + "$PROMPT" +) + +exec "${BWRAP_ARGS[@]}" diff --git a/scripts/post-slack-summary/run.sh b/scripts/post-slack-summary/run.sh new file mode 100755 index 0000000..0cf602a --- /dev/null +++ b/scripts/post-slack-summary/run.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Generate the weekly Slack activity summary for icon4py-knowledge in a sandbox +# and post it to Slack. Single entry point for both local testing and CI. +# +# Requires: +# - pi and bwrap installed and on PATH +# - CSCS_INFERENCE_API_KEY exported (local auth, or auth.json in the skill dir) +# - GITHUB_TOKEN exported (read-only, for gh CLI inside the sandbox) +# - SLACK_WEBHOOK_URL exported (to post the summary) +# +# Auth: pi authenticates from auth.json in the skill config dir (local testing) +# or CSCS_INFERENCE_API_KEY (CI, forwarded into the sandbox). run.sh does not +# check auth; pi fails loudly if neither is available. + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +SKILL_DIR="$REPO_ROOT/.github/workflows/weekly-slack-summary" +SUMMARY_FILE="$REPO_ROOT/weekly_slack_summary.md" + +command -v pi >/dev/null || { echo "Error: pi not found on PATH." >&2; exit 1; } +command -v bwrap >/dev/null || { echo "Error: bwrap not found on PATH." >&2; exit 1; } +pi --version +bwrap --version | head -1 + +if [[ -z "${GITHUB_TOKEN:-}" ]]; then + echo "Error: GITHUB_TOKEN is not set." >&2 + exit 1 +fi + +if [[ -z "${SLACK_WEBHOOK_URL:-}" ]]; then + echo "Error: SLACK_WEBHOOK_URL is not set." >&2 + exit 1 +fi + +# The summary is written through a temp file bound over the repo path so the +# sandbox can write exactly one file without write access to the repo root. +TMP_SUMMARY="$(mktemp)" +touch "$SUMMARY_FILE" +trap 'rm -f "$TMP_SUMMARY"' EXIT + +echo "Generating weekly summary in sandbox..." +PI_SANDBOX_CHDIR="$REPO_ROOT" \ +PI_SANDBOX_EXTRA_BINDS="$TMP_SUMMARY;$SUMMARY_FILE" \ +PI_SANDBOX_ENABLE_GITHUB_TOKEN=1 \ +CSCS_INFERENCE_API_KEY="${CSCS_INFERENCE_API_KEY:-}" \ +GITHUB_TOKEN="$GITHUB_TOKEN" \ + "$REPO_ROOT/scripts/pi-sandboxed.sh" \ + "$SKILL_DIR" \ + "$SKILL_DIR" \ + "Generate the weekly Slack activity summary for icon4py-knowledge and write it to weekly_slack_summary.md" + +if [[ ! -s "$SUMMARY_FILE" ]]; then + echo "Error: missing or empty summary: $SUMMARY_FILE" >&2 + exit 1 +fi + +echo "Posting summary to Slack..." +"$REPO_ROOT/scripts/post-slack-summary.py" "$SUMMARY_FILE" +echo "Done." From 17317947780a73564949cb2583b424b6561fffef Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Mon, 10 Aug 2026 17:17:13 +0200 Subject: [PATCH 2/4] Make bwrap work --- .github/workflows/icon4py-weekly-review.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/icon4py-weekly-review.yml b/.github/workflows/icon4py-weekly-review.yml index 6ec5acb..d3c5b26 100644 --- a/.github/workflows/icon4py-weekly-review.yml +++ b/.github/workflows/icon4py-weekly-review.yml @@ -42,6 +42,18 @@ jobs: sudo apt-get update sudo apt-get install -y bubblewrap + - name: Enable unprivileged user namespaces + run: | + # Ubuntu 24.04 (the current ubuntu-latest image) restricts + # unprivileged user namespaces via AppArmor + # (kernel.apparmor_restrict_unprivileged_userns=1), so bwrap fails + # with "setting up uid map: Permission denied" because the + # bubblewrap package ships no AppArmor profile. Relax the + # restriction for this job. See + # https://ubuntu.com/blog/ubuntu-23-10-restricted-unprivileged-user-namespaces + sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true + sudo sysctl -w kernel.unprivileged_userns_clone=1 || true + - name: Install uv uses: astral-sh/setup-uv@v6 From 6fdd98a0be39d431041c4831e29958dfde3ee8c6 Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Mon, 10 Aug 2026 18:42:12 +0200 Subject: [PATCH 3/4] ci: create the review branch before committing review findings commit-and-pr.sh committed to HEAD and then pushed a branch ref that was never created. In CI the checkout is detached (pull_request event), so the commit landed on detached HEAD and the push failed with 'src refspec ... does not match any'. Check out the branch first. --- scripts/icon4py-review/commit-and-pr.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/icon4py-review/commit-and-pr.sh b/scripts/icon4py-review/commit-and-pr.sh index beef821..fcf01a9 100755 --- a/scripts/icon4py-review/commit-and-pr.sh +++ b/scripts/icon4py-review/commit-and-pr.sh @@ -38,6 +38,10 @@ fi week=$(date +%G-W%V) branch="review/week-${week}-$(date +%s%N)" +# The commit lands on HEAD, which is detached in CI (actions/checkout on a +# pull_request event), so create the branch first or the push below has +# nothing to push. +git checkout -b "$branch" git -c user.name="icon4py-review-bot" \ -c user.email="icon4py-review-bot@users.noreply.github.com" \ commit -m "review(week ${week}): update icon4py findings" From b5a01e906c98a383e90d8db812777d27556bb5f2 Mon Sep 17 00:00:00 2001 From: icon4py-review-bot Date: Mon, 10 Aug 2026 20:58:31 +0200 Subject: [PATCH 4/4] review(week 2026-W33): update icon4py findings --- content/review/index.md | 6 +- .../issues/icon4py-2026-08-10-1440baa.md | 47 +++++++++ .../issues/icon4py-2026-08-10-89ecd75.md | 46 +++++++++ .../issues/icon4py-2026-08-10-8de4796.md | 46 +++++++++ content/review/reports/2026-08-10-1905.md | 98 +++++++++++++++++++ 5 files changed, 241 insertions(+), 2 deletions(-) create mode 100644 content/review/issues/icon4py-2026-08-10-1440baa.md create mode 100644 content/review/issues/icon4py-2026-08-10-89ecd75.md create mode 100644 content/review/issues/icon4py-2026-08-10-8de4796.md create mode 100644 content/review/reports/2026-08-10-1905.md diff --git a/content/review/index.md b/content/review/index.md index 66dd8c7..8b311a8 100644 --- a/content/review/index.md +++ b/content/review/index.md @@ -8,7 +8,9 @@ This page tracks automated review findings for C2SM/icon4py. Individual issue fi ## Open issues -_No open issues._ +- [[review/issues/icon4py-2026-08-10-1440baa.md|Global reductions issue a redundant MPI.Allreduce and device synchronization on every min/max/sum/mean call]] - severity: high, tags: allreduce, global-reduction, gpu, hot-path, mpi, synchronization +- [[review/issues/icon4py-2026-08-10-89ecd75.md|PPM4GPU integer vertical tracer flux sums out-of-bounds K levels at the top and bottom]] - severity: high, tags: k-offset, out-of-bounds, ppm, tracer-advection, vertical-advection +- [[review/issues/icon4py-2026-08-10-8de4796.md|PPM4GPU fractional vertical tracer flux sums out-of-bounds K levels at the top and bottom]] - severity: high, tags: k-offset, out-of-bounds, ppm, tracer-advection, vertical-advection ## Fixed issues @@ -20,4 +22,4 @@ _No invalid issues._ ## Reports -_No reports._ +- [[review/reports/2026-08-10-1905.md|Weekly icon4py review 2026-08-10]] diff --git a/content/review/issues/icon4py-2026-08-10-1440baa.md b/content/review/issues/icon4py-2026-08-10-1440baa.md new file mode 100644 index 0000000..8d75822 --- /dev/null +++ b/content/review/issues/icon4py-2026-08-10-1440baa.md @@ -0,0 +1,47 @@ +--- +id: icon4py-2026-08-10-1440baa +title: Global reductions issue a redundant MPI.Allreduce and device synchronization + on every min/max/sum/mean call +issue_status: open +severity: high +confidence: high +fingerprint: performance:model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py:GlobalReductions._calc_buffer_size:redundant-global-allreduce +tags: +- allreduce +- global-reduction +- gpu +- hot-path +- mpi +- synchronization +created: '2026-08-10' +updated: '2026-08-10' +last_seen: '2026-08-10' +source: + repo: C2SM/icon4py + ref: main + commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351 + file: model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py + lines: + - 403 + - 453 + symbol: GlobalReductions._calc_buffer_size +found_by: +- icon4py-performance-reviewer +run_id: weekly-2026-W33 +history: +- date: '2026-08-10' + event: detected + run_id: weekly-2026-W33 + commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351 +--- +## Summary + +In the distributed (MPI) reduction implementation, every call to `GlobalReductions.min`, `.max`, `.sum`, and `.mean` invokes `_calc_buffer_size`, which itself performs a full `_reduce` — i.e. a second `mpi4py.MPI.Allreduce` plus, on a GPU backend, a `cuda.runtime.deviceSynchronize()`. The result is used only to detect the degenerate globally-empty-buffer case and raise `ValueError`. In normal production runs the buffer is never empty, so each global reduction does 2 `MPI.Allreduce` calls and 2 device-wide synchronizations instead of 1 + 1. This directly hits the per-timestep hot path: `Icon4pyDriver._adjust_ndyn_substeps_var` calls `self.global_reductions.max(...)` once per timestep whenever `nonhydrostatic` is configured (the default dycore), over a 0-d scalar (`max_vertical_cfl[()]`), so the second `Allreduce` is over a single integer every timestep. Global allreduce is a synchronizing collective and a primary MPI scaling bottleneck; the extra `deviceSynchronize()` additionally drains the GPU pipeline and defeats kernel overlap/concurrency at scale. + +## Evidence + +`_calc_buffer_size` (mpi_decomposition.py:403-408) returns `self._reduce(array_ns.asarray([buffer.size]), array_ns.sum, mpi4py.MPI.SUM)`, i.e. it triggers a second `self.process_props.comm.Allreduce(...)` preceded by `array_ns.cuda.runtime.deviceSynchronize()` (mpi_decomposition.py:399-401). `min`/`max`/`sum` only consume this value as `if self._calc_buffer_size(buffer) == 0: raise ValueError(...)` (mpi_decomposition.py:413, 424, 435), so in the non-empty case the second collective is computed and discarded. `mean` (mpi_decomposition.py:446) reuses `global_buffer_size` as a divisor but still runs it as a separate collective instead of folding the count into the reduction. Hot-path caller `standalone_driver.py:452` runs `global_max_vertical_cfl = self.global_reductions.max(...)` every timestep via `_adjust_ndyn_substeps_var`, which `time_integration` calls whenever `self.config.nonhydrostatic is not None`. The single-node `SingleNodeReductions` has no such redundant collective, confirming the cost is specific to production MPI/GPU runs. + +## Suggested fix + +Skip the global size check on the common non-empty path. For `min`/`max`/`sum`, only compute a global non-empty flag when the local buffer is already empty, e.g. guard with `if buffer.size == 0:` and then (and only then) reduce a single `int(buffer.size > 0)` with `MPI.MAX`/`MPI.SUM` to decide whether to raise `ValueError`. Alternatively, cache the global owned count per dimension/shape, computed once, instead of recomputing it via an `Allreduce` on every call. For `mean`, which legitimately needs the count as divisor, either cache the count or fuse `sum` and `count` into a single `Allreduce` over a 2-element buffer with a custom reduction op, instead of issuing two separate collectives. These changes halve the number of global collectives and remove the avoidable `deviceSynchronize()` from the per-timestep reduction while preserving the existing all-empty `ValueError` behaviour. diff --git a/content/review/issues/icon4py-2026-08-10-89ecd75.md b/content/review/issues/icon4py-2026-08-10-89ecd75.md new file mode 100644 index 0000000..dd51cec --- /dev/null +++ b/content/review/issues/icon4py-2026-08-10-89ecd75.md @@ -0,0 +1,46 @@ +--- +id: icon4py-2026-08-10-89ecd75 +title: PPM4GPU integer vertical tracer flux sums out-of-bounds K levels at the top + and bottom +issue_status: open +severity: high +confidence: high +fingerprint: correctness:model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_integer_flux.py:_sum_neighbor_contributions_all:oob-k-offset-read +tags: +- k-offset +- out-of-bounds +- ppm +- tracer-advection +- vertical-advection +created: '2026-08-10' +updated: '2026-08-10' +last_seen: '2026-08-10' +source: + repo: C2SM/icon4py + ref: main + commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351 + file: model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_integer_flux.py + lines: + - 34 + - 57 + symbol: _sum_neighbor_contributions_all +found_by: +- icon4py-correctness-reviewer +run_id: weekly-2026-W33 +history: +- date: '2026-08-10' + event: detected + run_id: weekly-2026-W33 + commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351 +--- +## Summary + +`_sum_neighbor_contributions_all` builds the integer-flux contribution by selecting `p_cc(dims.KDim + n) * p_cellmass_now(dims.KDim + n)` for `n = 0..4` upward and `p_cc(dims.KDim - (n+1)) * p_cellmass_now(dims.KDim - (n+1))` for `n = 0..4` downward, each gated by `where(mask1 & js_gtN, …, 0.0)` / `where(mask2 & js_gtN, …, 0.0)` (lines 34-43) with `js = floor(abs(z_cfl)) - 1`. The stencil runs over `KDim = (1, num_levels)` (`tracer_advection_vertical.PiecewiseParabolicMethod`), but `p_cc` (the tracer field) and `p_cellmass_now` (airmass/rhodz) are allocated with only `num_levels` K levels and no extension (`tracer_states.initialize_tracer_state`, `tracer_advection_states.AdvectionDiagnosticState`). At `k = num_levels - 1` the upward reads `p_cc(dims.KDim + 1..4)` access indices `num_levels..num_levels+3` (past the end); at `k = 1` the downward reads `p_cc(dims.KDim - 2..5)` access indices `-1..-4` (before the start). Because `js_gtN` is `True` whenever `|vertical CFL| > N + 1`, these out-of-bounds values are selected and added into `prod_jks` (lines 50-56) and hence into `z_iflx`/`p_upflux` (lines 84-88), corrupting the vertical tracer mass flux at the top and bottom levels. On the `roundtrip`/embedded backend the positive out-of-bounds read raises `IndexError` (crash); on `gtfn_cpu` the negative index wraps/reads adjacent memory and the positive index reads adjacent or past-array memory (wrong values). The stencil is annotated `# TODO(dastrm): this stencil has no test` and `# this stencil does not strictly match the fortran code`. + +## Evidence + +The dycore guards the same K-neighbour pattern with `concat_where` plus a K-extended `w` field (`compute_advection_in_vertical_momentum_equation`, `prognostic_state.initialize_prognostic_state`), confirming that plain `where` evaluates both branches and that the `dims.KDim ± n` accesses here are out of bounds for a non-extended field. The stencil is driven with `vertical_start=1, vertical_end=num_levels`, so `k = num_levels - 1` (and `k = 1`) are always reached. The integration test (`test_standalone_driver.py`) uses a balanced reference with small boundary vertical mass flux (`|CFL| < 2`, so `js_gt1` is `False` and the reads are masked) and stays bit-exact; the corruption appears only during strong vertical convection, which is routine with `ndyn_substeps > 1` because the tracer step uses the full physics time step with the dycore substep-averaged mass flux. + +## Suggested fix + +Use `concat_where` keyed on `slev <= k <= nlev - 1 - n` (and the symmetric lower bound for the downward branch) so each `p_cc(dims.KDim ± n)` / `p_cellmass_now(dims.KDim ± n)` access is evaluated only where it is in bounds, matching the dycore's handling. Alternatively extend the tracer and airmass/rhodz fields by enough K levels to cover the maximum `±5` offset and ensure the selection default reproduces ICON's boundary behaviour. diff --git a/content/review/issues/icon4py-2026-08-10-8de4796.md b/content/review/issues/icon4py-2026-08-10-8de4796.md new file mode 100644 index 0000000..0d14c20 --- /dev/null +++ b/content/review/issues/icon4py-2026-08-10-8de4796.md @@ -0,0 +1,46 @@ +--- +id: icon4py-2026-08-10-8de4796 +title: PPM4GPU fractional vertical tracer flux sums out-of-bounds K levels at the + top and bottom +issue_status: open +severity: high +confidence: high +fingerprint: correctness:model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_fractional_flux.py:_sum_neighbor_contributions:oob-k-offset-read +tags: +- k-offset +- out-of-bounds +- ppm +- tracer-advection +- vertical-advection +created: '2026-08-10' +updated: '2026-08-10' +last_seen: '2026-08-10' +source: + repo: C2SM/icon4py + ref: main + commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351 + file: model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_fractional_flux.py + lines: + - 33 + - 55 + symbol: _sum_neighbor_contributions +found_by: +- icon4py-correctness-reviewer +run_id: weekly-2026-W33 +history: +- date: '2026-08-10' + event: detected + run_id: weekly-2026-W33 + commit_sha: e68ed9505f5eb69b242f9a33f62ded447a52c351 +--- +## Summary + +`_sum_neighbor_contributions` selects `p_cc(dims.KDim + n)` for `n = 0..4` upward and `p_cc(dims.KDim - (n+1))` for `n = 0..4` downward, each gated by `where(mask1 & js_eqN, …, 0.0)` / `where(mask2 & js_eqN, …, 0.0)` (lines 33-42) with `js = floor(abs(z_cfl))`. The stencil runs over `KDim = (1, num_levels)` (`tracer_advection_vertical.PiecewiseParabolicMethod`), but `p_cc` (tracer) and `p_cellmass_now` (airmass/rhodz) are allocated with only `num_levels` K levels and no extension (`tracer_states.initialize_tracer_state`, `tracer_advection_states.AdvectionDiagnosticState`). At `k = num_levels - 1` the upward reads `p_cc(dims.KDim + 1..4)` access indices `num_levels..num_levels+3` (past the end); at `k = 1` the downward reads `p_cc(dims.KDim - 2..5)` access indices `-1..-4` (before the start). Because `js_eqN` is `True` whenever `floor(|vertical CFL|) == N`, these out-of-bounds values are selected and summed into `p_cc_jks` (lines 44-54), then into `z_q_int` and the fractional flux `p_upflux` (lines 84-97), corrupting the vertical tracer flux at the top and bottom levels. On the `roundtrip`/embedded backend the positive out-of-bounds read raises `IndexError` (crash); on `gtfn_cpu` the negative index wraps/reads adjacent memory and the positive index reads adjacent or past-array memory (wrong values). The stencil is annotated `# TODO(dastrm): this stencil has no test` and `# this stencil does not strictly match the fortran code`. + +## Evidence + +As with the integer-flux and Courant-number cases, the dycore guards the identical pattern with `concat_where` and a K-extended `w` field (`compute_advection_in_vertical_momentum_equation`, `prognostic_state.initialize_prognostic_state`), confirming that plain `where` reads out of bounds here. The stencil is driven with `vertical_start=1, vertical_end=num_levels`, so both boundary levels are reached. The balanced integration reference (`|vertical CFL| < 2`, so `js_eq1..4` are `False` at the boundaries) masks the bug and keeps the comparison bit-exact; it surfaces only during strong vertical convection, which is routine with `ndyn_substeps > 1` because the tracer step uses the full physics time step with the dycore substep-averaged mass flux. + +## Suggested fix + +Use `concat_where` keyed on `slev <= k <= nlev - 1 - n` (and the symmetric lower bound for the downward branch) for each `p_cc(dims.KDim ± n)` access, matching the dycore. Alternatively extend the tracer and airmass/rhodz fields by enough K levels to cover the maximum `±5` offset and align the selection default with ICON's boundary behaviour. diff --git a/content/review/reports/2026-08-10-1905.md b/content/review/reports/2026-08-10-1905.md new file mode 100644 index 0000000..94f56dd --- /dev/null +++ b/content/review/reports/2026-08-10-1905.md @@ -0,0 +1,98 @@ +--- +title: "Weekly icon4py review 2026-08-10" +tags: +- review +created: 2026-08-10 +icon4py_commit: e68ed9505f5eb69b242f9a33f62ded447a52c351 +--- + +# Weekly icon4py review — 2026-08-10 + +## Run metadata + +| Field | Value | +| --- | --- | +| Review date | 2026-08-10 | +| Run ID | weekly-2026-W33 | +| icon4py commit | `e68ed9505f5eb69b242f9a33f62ded447a52c351` | +| icon4py checkout | `/home/runner/work/icon4py-knowledge/icon4py-knowledge/icon4py-checkout` | +| Requested severity | high | +| Open issues at start | 0 (open-issues.json was empty — effectively a first-run review; no `changes.diff` baseline) | + +## Summary counts + +| Count | Value | +| --- | --- | +| Total findings reviewed | 4 | +| Findings submitted to the skeptic panel | 4 | +| New accepted findings | 3 | +| Merged duplicates | 0 | +| Rejected findings | 1 | +| Uncertain / ambiguous findings | 0 | +| Existing open issues assessed for fixedness | 0 (none open) | + +Reviewer breakdown: `icon4py-correctness-reviewer` produced 3 findings, `icon4py-performance-reviewer` produced 1 finding. All 4 were submitted to a 3-skeptic panel (12 votes total). No skeptic voted `DUPLICATE` (the open-issues tracker was empty), so no duplicates were merged this run. + +## New accepted findings + +Written to `/tmp/icon4py-review-findings/accepted.json`. All three were confirmed by a unanimous (3× `PASS`) panel, so `confidence` = `high`. + +| Fingerprint | Title | Severity | Confidence | File | +| --- | --- | --- | --- | --- | +| `correctness:.../stencils/compute_ppm4gpu_integer_flux.py:_sum_neighbor_contributions_all:oob-k-offset-read` | PPM4GPU integer vertical tracer flux sums out-of-bounds K levels at the top and bottom | high | high | `model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_integer_flux.py` | +| `correctness:.../stencils/compute_ppm4gpu_fractional_flux.py:_sum_neighbor_contributions:oob-k-offset-read` | PPM4GPU fractional vertical tracer flux sums out-of-bounds K levels at the top and bottom | high | high | `model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_fractional_flux.py` | +| `performance:.../decomposition/mpi_decomposition.py:GlobalReductions._calc_buffer_size:redundant-global-allreduce` | Global reductions issue a redundant MPI.Allreduce and device synchronization on every min/max/sum/mean call | high | high | `model/common/src/icon4py/model/common/decomposition/mpi_decomposition.py` | + +### Notes on the accepted findings +- **PPM4GPU integer & fractional vertical tracer flux (2 correctness findings).** `_sum_neighbor_contributions_all` (`compute_ppm4gpu_integer_flux.py`) and `_sum_neighbor_contributions` (`compute_ppm4gpu_fractional_flux.py`) read the non-K-extended tracer (`p_cc`) and airmass/rhodz (`p_cellmass_now`) fields at K offsets of up to ±5, gated only by CFL-based masks (`js_gtN` / `js_eqN`) with no K-bounds guard. At the top (`k = num_levels - 1`) and bottom (`k = 1`) levels these reads go out of bounds and are *selected* (not masked) when the vertical CFL is large — the PPM scheme's intended operating regime, routinely reachable with `ndyn_substeps > 1`. The panel confirmed the defect using the codebase's own contrast: the sibling `compute_ppm4gpu_courant_number` and the dycore guard the identical K-neighbour pattern with `in_bounds_pN` / `concat_where` plus a K-extended `w`. The integration test stays bit-exact only because its balanced reference keeps the boundary CFL below the triggering threshold, so the bug is latent. +- **Redundant global reduction (1 performance finding).** `GlobalReductions._calc_buffer_size` performs a full second `MPI.Allreduce` (plus a `cuda.runtime.deviceSynchronize()` on GPU) on every `min`/`max`/`sum`/`mean` call solely to detect the globally-empty case. In production the buffer is never empty, so each call does 2 collectives + 2 device syncs instead of 1 + 1, hitting the per-timestep hot path via `_adjust_ndyn_substeps_var`. The panel noted the primary suggested fix (`if buffer.size == 0:`) has a subtle MPI-deadlock risk under mixed-emptiness (Allreduce requires uniform participation), but the finding also proposes sound alternatives (cache the per-dimension global owned count once; or fuse `sum`+`count` into a single `Allreduce` for `mean`), so a correct low-risk fix exists. + +## Merged duplicates + +Written to `/tmp/icon4py-review-findings/duplicates.json`. + +None this run (open-issues.json was empty, so no `DUPLICATE` verdicts were possible). + +| New finding title | Existing issue ID | Confidence | +| --- | --- | --- | + +## Rejected findings + +| Title | Skeptic verdicts (×3) | File | Note | +| --- | --- | --- | --- | +| PPM4GPU vertical Courant number selects out-of-bounds K levels at the model bottom | REJECT, REJECT, REJECT | `model/atmosphere/tracer_advection/src/icon4py/model/atmosphere/tracer_advection/stencils/compute_ppm4gpu_courant_number.py` | The panel found the finding's central premise incorrect. The stencil is set up with `nlev = self._grid.num_levels - 1` (the 0-indexed bottom level), **not** `num_levels`, so at the model bottom `in_bounds_p0 = k <= nlev - 1` is **False** by construction. The `mass_gt_cellmass_pN` flags are chained (via `&`) to `in_bounds_pN`, so they are all `False` at the bottom regardless of the CFL magnitude, and the out-of-bounds `p_cellmass_now` value is **never selected** — the stencil already produces the correct saturated Courant number (~1.0). The finding's predicted wrong value (~2.0) and the embedded-backend crash claim rest on this false premise. Notably, `compute_ppm4gpu_courant_number` is itself the safe, bounds-guarded pattern that the two accepted flux findings contrast against; this finding flagged the one stencil in the group that is actually guarded correctly. | + +## Uncertain / ambiguous findings + +None. + +| Title | Verdicts | Note | +| --- | --- | --- | + +## Fixedness outcomes + +Written to `/tmp/icon4py-review-findings/fixedness.json`. + +No existing open issues (open-issues.json was empty), so no fixedness assessment was performed and no `icon4py-fixedness-checker` panel was spawned. + +| Issue ID | Verdict | Note | +| --- | --- | --- | + +## Failures and caveats + +- **Reviewers:** No failures. Both `icon4py-correctness-reviewer` and `icon4py-performance-reviewer` completed successfully and wrote valid JSON to `/tmp/icon4py-review-findings/correctness.json` and `performance.json`. +- **Skeptics:** No failures. All 12 `icon4py-finding-skeptic` subagents (3 per finding × 4 findings) completed and wrote valid verdict JSON. All votes were located and counted. Minor housekeeping note: one skeptic for the integer-flux finding wrote its `3.json` under a slightly different (de-duplicated) directory name than the other two; the content was found, read, and counted normally. Vote files used a filesystem-safe form of each fingerprint (with path separators and colons normalized to `_`) under `findings_dir/votes/`. +- **Fixedness:** No failures (nothing to assess; no existing open issues). +- **No duplicate merges** were possible because the open-issues tracker was empty at the start of the run. +- Findings were passed to the skeptic panel via per-finding files under `/tmp/icon4py-review-findings/findings/` (each enriched with the originating `reviewer`); the accepted findings in `accepted.json` carry `reviewer`, `confidence` (`high`), and `tags`. + +## Outputs + +| Artifact | Path | +| --- | --- | +| Correctness findings | `/tmp/icon4py-review-findings/correctness.json` | +| Performance findings | `/tmp/icon4py-review-findings/performance.json` | +| Accepted (new) findings | `/tmp/icon4py-review-findings/accepted.json` | +| Duplicate merges | `/tmp/icon4py-review-findings/duplicates.json` | +| Fixedness outcomes | `/tmp/icon4py-review-findings/fixedness.json` | +| Overview report | `/tmp/review-reports/2026-08-10-1905.md` |