[PR] fix(security-tracker-stats): retry 5xx and default cache to temp root [magpie]

["sharpca (via GitHub)" <gi...@apache.org>]

sharpca opened a new pull request, #1752:
URL: https://github.com/apache/magpie/pull/1752

   ## Summary
   
   - `fetch_issues.py` retried nothing: a single transient `HTTP 502` from the API
     became a `SystemExit`, and since this fetch is the first of six steps the
     operator had to restart the whole dashboard run by hand. `run_gh` now retries
     the failures that can clear on their own (5xx, gateway timeouts, connection
     resets) with exponential backoff, and still fails fast on an auth error, a bad
     repo slug, or an unknown flag.
   - The default cache and output paths no longer hardcode `/tmp`. A sandboxed run
     is granted its own scratch directory and cannot write `/tmp` at all, so the
     first fetch step failed before it ever reached the API. Both defaults now
     derive from `tempfile.gettempdir()` (`$TMPDIR` on POSIX, `%TEMP%` on Windows);
     `TRACKER_STATS_CACHE` and `TRACKER_STATS_OUT` still win when set.
   - README, `run.sh`, the skill prose, the adopter template, and the step-2 eval
     system prompt follow the new default.
   
   ## Type of change
   
   - [x] Python package (`tools/*/` with `pyproject.toml`)
   - [x] Documentation (`docs/`, `README.md`, `CONTRIBUTING.md`)
   - [x] Skill change — prose only. The `tracker-stats-dashboard` `SKILL.md`
         restates the cache default; no behaviour change, so no new fixture.
   
   ## Test plan
   
   - [x] For Python packages touched: `uv run pytest` / `ruff check` / `mypy` passes
     - `pytest`: 149 passed (142 existing + 7 new in `tests/test_fetch_retry.py`)
     - `ruff check .` and `ruff format --check .`: clean
     - `mypy`: no issues in 6 source files
   - [x] New tests in `tests/test_fetch_retry.py` cover transient-status detection,
         retry-then-succeed with a doubling backoff, give-up after the attempt
         limit, no retry on a 401, and both branches of the cache default.
   
   ## RFC-AI-0004 compliance
   
   - [x] **Sandbox** — removes two writes that fell outside the sandbox's writable
         paths (`/tmp/tracker-stats-cache`, `/tmp/airflow_s_monthly.html`); no new
         host access or network reach is introduced.
   
   ## Linked issues
   
   Closes #1744
   
   ## Notes for reviewers
   
   - `TRACKER_STATS_OUT` was not named in the issue, but it hardcoded `/tmp` for the
     same reason and would have kept the run failing after the cache was fixed.
     Happy to split it out if you would rather keep this PR to the cache alone.
   - The eval fixtures' `report.md` files keep `Cache directory: /tmp/...`: on a
     POSIX host without `TMPDIR`, `tempfile.gettempdir()` is `/tmp`, so that output
     is still exactly what the new default produces.
   - Only 5xx / gateway-timeout / connection-reset count as transient. A 403
     secondary-rate-limit is deliberately not retried — it needs the operator to
     read the message, not a shorter backoff.
   
   ## Generative AI disclosure
   
   This change was authored with an AI coding agent (WorkBuddy AI, DeepSeek-V4.1-Flash)
   under human review. The commit carries the `Generated-by:` trailer; the
   contributor has reviewed the diff and owns the change.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@magpie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(security-tracker-stats): retry 5xx and default cache to temp root [magpie]

["potiuk (via GitHub)" <gi...@apache.org>]

potiuk merged PR #1752:
URL: https://github.com/apache/magpie/pull/1752


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@magpie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(security-tracker-stats): retry 5xx and default cache to temp root [magpie]

["Kaap10 (via GitHub)" <gi...@apache.org>]

Kaap10 commented on code in PR #1752:
URL: https://github.com/apache/magpie/pull/1752#discussion_r4339917201


##########
tools/security-tracker-stats-dashboard/fetch_issues.py:
##########
@@ -21,49 +21,109 @@
 
 import json
 import os
+import re
 import subprocess
+import sys
+import tempfile
+import time
 
-ROOT = os.environ.get("TRACKER_STATS_CACHE", "/tmp/tracker-stats-cache")
+# The cache defaults under the platform temp root rather than a hardcoded `/tmp`:
+# a sandboxed run is typically granted its own scratch directory and cannot write
+# `/tmp`, so `/tmp/tracker-stats-cache` made the first fetch step fail outright.
+# `tempfile.gettempdir()` honours `$TMPDIR` (POSIX) and `%TEMP%` (Windows).
+ROOT = os.environ.get("TRACKER_STATS_CACHE") or os.path.join(tempfile.gettempdir(), "tracker-stats-cache")
 REPO = os.environ.get("TRACKER_STATS_REPO", "airflow-s/airflow-s")
 
-os.makedirs(ROOT, exist_ok=True)
-
-print(f"Fetching issue list from {REPO} (state=all, limit 1000) ...")
-r = subprocess.run(
-    [
-        "gh",
-        "issue",
-        "list",
-        "--repo",
-        REPO,
-        "--state",
-        "all",
-        "--limit",
-        "1000",
-        "--json",
-        # `updatedAt` is consumed by fetch_events.py, which refuses to trust a cached
-        # events file older than the issue's last update. Dropping it here silently
-        # disables that guard, because the check falls back to "no timestamp -> trust
-        # the cache" and every relabelled issue keeps its stale event history.
-        # `body` and `closedByPullRequestsReferences` ride on this one paginated call
-        # so fetch_bodies.py can build issue_extra.json without a `gh issue view`
-        # per issue.
-        "number,title,state,stateReason,createdAt,closedAt,updatedAt,labels,comments,body,closedByPullRequestsReferences",
-    ],
-    capture_output=True,
-    text=True,
-    timeout=300,
+# `gh` reports a server-side hiccup as a non-zero exit with the status in stderr.
+# A single transient `HTTP 502` on the first call used to abort the entire
+# dashboard run, so retry the failures that are worth retrying and fail fast on
+# everything else (auth errors, a bad repo slug, an unknown flag).
+_TRANSIENT_FAILURE = re.compile(
+    r"HTTP\s+5\d\d"
+    r"|Bad Gateway"
+    r"|Service Unavailable"
+    r"|Gateway Time-?out"
+    r"|Internal Server Error"
+    r"|connection reset"
+    r"|timed out",
+    re.IGNORECASE,
 )
-if r.returncode != 0:
-    raise SystemExit(f"gh failed: {r.stderr}")
-
-issues = json.loads(r.stdout)
-with open(f"{ROOT}/issues.json", "w") as f:
-    json.dump(issues, f)
-
-print(f"Wrote {len(issues)} issues to {ROOT}/issues.json")
-if len(issues) >= 1000:
-    print(
-        "WARNING: the issue list hit the 1000-issue cap; older issues are missing "
-        "and every count in the dashboard is a floor. Raise --limit in fetch_issues.py."
+
+ATTEMPTS = 3
+BASE_DELAY_SECONDS = 2.0
+
+
+def is_transient_failure(stderr: str) -> bool:
+    """True when a failed `gh` call looks like a retryable server-side error."""
+    return bool(_TRANSIENT_FAILURE.search(stderr or ""))
+
+
+def run_gh(
+    args: list[str],
+    *,
+    attempts: int = ATTEMPTS,
+    base_delay: float = BASE_DELAY_SECONDS,
+    timeout: int = 300,
+) -> subprocess.CompletedProcess:
+    """Run `gh`, retrying transient failures with exponential backoff.
+
+    Raises `SystemExit` with the last stderr once the attempts are exhausted, or

Review Comment:
   `_TRANSIENT_FAILURE` regex includes `"timed out"`, which matches if `gh` itself outputs a timeout to stderr. However, if the command hits the 300s subprocess limit, `subprocess.run` raises `subprocess.TimeoutExpired` instead of returning a completed process.
   
   Consider wrapping `subprocess.run` to catch `TimeoutExpired` and treat it as transient so it can also be retried:
   
   ```suggestion
           try:
               r = subprocess.run(args, capture_output=True, text=True, timeout=timeout)
           except subprocess.TimeoutExpired as exc:
               if attempt == attempts:
                   raise SystemExit(f"gh timed out after {timeout}s: {exc}")
               delay = base_delay * 2 ** (attempt - 1)
               print(
                   f"gh timed out (attempt {attempt}/{attempts}), retrying in {delay:g}s",
                   file=sys.stderr,
               )
               time.sleep(delay)
               continue
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@magpie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(security-tracker-stats): retry 5xx and default cache to temp root [magpie]

["Kaap10 (via GitHub)" <gi...@apache.org>]

Kaap10 commented on code in PR #1752:
URL: https://github.com/apache/magpie/pull/1752#discussion_r4339928118


##########
plugins/magpie-security/skills/tracker-stats-dashboard/SKILL.md:
##########
@@ -207,7 +207,8 @@ If the adopter passes nothing, surface the resolved output path and cache state
 2. **Check cache freshness.** Inspect
    `<cache>/issues.json` mtime, where `<cache>` is the
    `tracker_stats_cache` value from the step 1 config (else
-   `${TRACKER_STATS_CACHE:-/tmp/tracker-stats-cache}`, the fetch scripts' default).
+   `${TRACKER_STATS_CACHE:-<platform temp dir>/tracker-stats-cache}`, the
+   fetch scripts' default — `$TMPDIR` on POSIX, `%TEMP%` on Windows).

Review Comment:
   Because prose in Step 2 was modified here, the frontmatter `surface_hash` and `measured_tokens` (lines 19–21) are now out of date.
   
   Running `prek run --all-files` (or `python tools/dev/check-measured-tokens.py --fix`) will recalculate and update these stamps so the CI `skill-surface-hash` hook passes.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@magpie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(security-tracker-stats): retry 5xx and default cache to temp root [magpie]

["Kaap10 (via GitHub)" <gi...@apache.org>]

Kaap10 commented on code in PR #1752:
URL: https://github.com/apache/magpie/pull/1752#discussion_r4339934406


##########
tools/security-tracker-stats-dashboard/run.sh:
##########
@@ -55,4 +55,4 @@ echo "-> fetch_prs"
 echo "-> render"
 "${PY_CMD[@]}" "$HERE/render.py"
 
-echo "done: ${TRACKER_STATS_OUT:-/tmp/airflow_s_monthly.html}"
+echo "done: ${TRACKER_STATS_OUT:-${TMPDIR:-/tmp}/airflow_s_monthly.html}"

Review Comment:
   Small consistency nit: `render.py` resolves the path via Python's `tempfile.gettempdir()`, which maps to `%TEMP%` on Windows, whereas bash's `${TMPDIR:-/tmp}` will fallback to `/tmp`.
   
   Not a blocker on POSIX, but worth keeping in mind or letting `render.py` print the exact final path it wrote to.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@magpie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org