dilim/hermes-agent
1
0
Fork 0
mirror of https://github.com/NousResearch/hermes-agent.git synced 2026-08-09 15:20:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(ci): follow artifact download redirect without auth

Browse Source 
The artifact download URL returns a 302 redirect to a signed blob URL.
urllib sent the Authorization header to the blob, and the blob rejected it
with a 401 error. The download now has two hops. The first hop authenticates
to the API. The second hop follows the redirect without the auth header.

The query runs?event=workflow_call returns nothing for this repository.
GitHub flattens reusable-workflow jobs and their artifacts into the caller
run. The fetch now lists the artifacts on the orchestrator run only. The
dead sub-run enumeration is gone. Two API calls per cycle are gone with it.

The 'artifact statuses updated' reason never appeared. The code updated the
count before the comparison. Now the code compares first and updates after.

The code rejects zip members that contain '..' or start with '/'.

tests/ci/test_live_comment.py is deleted. This repository does not keep
tests for CI infrastructure.
This commit is contained in:
ethernet
2026-08-05 10:26:21 -04:00
parent 1d7d0e41af
commit ee7c614eef
2 changed files with 85 additions and 288 deletions
Download Patch File Download Diff File Expand all files Collapse all files

166
scripts/ci/live_comment.py
View File

@@ -20,12 +20,12 @@ Architecture:
- :func:`find_comment_id` / :func:`upsert_comment` — thin API wrappers.
- :func:`fetch_all_review_statuses` — enumerates all
``review-status-*`` artifacts across the orchestrator run and all
sub-workflow runs, downloads each, parses the ``review_status=``
line from ``review-status.json``, and merges into one array.
Recomputed from source every poll cycle, so statuses appear as
soon as each job uploads its artifact.
- :func:`fetch_all_review_statuses` — lists all ``review-status-*``
artifacts on the orchestrator run (GitHub attaches reusable-workflow
artifacts to the caller run), downloads each, parses the
``review_status=`` line from ``review-status.json``, and merges into
one array. Recomputed from source every poll cycle, so statuses
appear as soon as each job uploads its artifact.
- :func:`run` — the polling loop. Calls the API, classifies,
fetches artifacts, assembles, upserts, sleeps, repeats. Before
@@ -280,32 +280,6 @@ def upsert_comment(
_REVIEW_STATUS_ARTIFACT_PREFIX = "review-status-"
def _list_run_ids(token: str, repo: str, run_id: str) -> list[str]:
"""Return the orchestrator run ID + all sub-workflow run IDs.
Sub-workflow runs (workflow_call) may not exist yet on the first few
polls — that's fine, they just won't be in the list.
"""
owner, repo_name = repo.split("/")
run_info = _api_request(
f"{API_BASE}/repos/{owner}/{repo_name}/actions/runs/{run_id}", token
)
created_at = run_info.get("created_at", "")
head_sha = run_info.get("head_sha", "")
run_ids = [run_id]
sub_runs = _api_get_paginated(
f"{API_BASE}/repos/{owner}/{repo_name}/actions/runs"
f"?head_sha={head_sha}&event=workflow_call&per_page=100",
token, list_key="workflow_runs",
)
sub_runs = [r for r in sub_runs if r.get("created_at", "") >= created_at]
run_ids.extend(str(r["id"]) for r in sub_runs)
return run_ids
def _list_artifacts(token: str, repo: str, run_id: str) -> list[dict]:
"""List artifacts for a given run (paginated)."""
owner, repo_name = repo.split("/")
@@ -315,6 +289,13 @@ def _list_artifacts(token: str, repo: str, run_id: str) -> list[dict]:
)
class _NoRedirectHandler(urllib.request.HTTPRedirectHandler):
"""Redirect handler that never follows — used to capture the Location."""
def redirect_request(self, *args, **kwargs):
return None
def _download_artifact(
token: str, repo: str, artifact: dict, dest_dir: Path,
) -> Path | None:
@@ -328,17 +309,33 @@ def _download_artifact(
if not archive_download_url:
return None
# The archive_download_url is an API URL that redirects to a S3 URL.
# Build the request with our auth headers so the redirect works.
req = urllib.request.Request(archive_download_url, headers={
"Authorization": f"Bearer {token}",
"Accept": "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
"User-Agent": "ci-live-comment",
})
# The archive_download_url is an API URL that 302s to a signed blob
# URL. Hop 1 authenticates to the API; hop 2 follows the redirect
# WITHOUT the Authorization header — the blob rejects a request that
# carries both a SAS token and an Authorization header (401).
opener = urllib.request.build_opener(_NoRedirectHandler)
location = ""
try:
opener.open(urllib.request.Request(archive_download_url, headers={
"Authorization": f"Bearer {token}",
"Accept": "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
"User-Agent": "ci-live-comment",
}), timeout=30)
except urllib.error.HTTPError as e:
location = e.headers.get("Location", "") if e.code == 302 else ""
except Exception:
location = ""
if not location:
return None
zip_path = dest_dir / f"{artifact['name']}.zip"
try:
with urllib.request.urlopen(req) as resp:
# No auth headers here; further redirects are safe to follow.
with urllib.request.urlopen(
urllib.request.Request(location, headers={"User-Agent": "ci-live-comment"}),
timeout=60,
) as resp:
zip_path.write_bytes(resp.read())
except Exception:
return None
@@ -347,6 +344,8 @@ def _download_artifact(
extract_dir.mkdir(parents=True, exist_ok=True)
try:
with zipfile.ZipFile(zip_path) as zf:
if any(".." in name or name.startswith("/") for name in zf.namelist()):
return None
zf.extractall(extract_dir)
except Exception:
return None
@@ -372,12 +371,13 @@ def _parse_status_file(status_file: Path) -> list[dict]:
def fetch_all_review_statuses(
token: str, repo: str, run_id: str,
) -> list[dict]:
"""Fetch and merge all review-status artifacts across all runs.
"""Fetch and merge all review-status artifacts from the run.
Enumerates artifacts with the ``review-status-`` prefix from the
orchestrator run and all sub-workflow runs (workflow_call). Downloads
each, parses the ``review-status.json`` inside, and merges into a
single flat array.
Lists artifacts with the ``review-status-`` prefix on the orchestrator
run, downloads each, parses the ``review-status.json`` inside, and
merges into a single flat array. GitHub attaches artifacts uploaded by
reusable workflow jobs to the caller run, so one listing covers every
status-producing job.
Returns the merged list of ``{source, results: [...]}`` objects.
Artifacts that don't exist yet or fail to parse are silently skipped.
@@ -386,37 +386,43 @@ def fetch_all_review_statuses(
temp_base = Path("/tmp/review-status-artifacts")
try:
run_ids = _list_run_ids(token, repo, run_id)
artifacts = _list_artifacts(token, repo, run_id)
except Exception:
return all_statuses
for rid in run_ids:
try:
artifacts = _list_artifacts(token, repo, rid)
except Exception:
rs_artifacts = [
a for a in artifacts
if a.get("name", "").startswith(_REVIEW_STATUS_ARTIFACT_PREFIX)
]
if not rs_artifacts:
return all_statuses
# Clean temp dir for this run's artifacts.
run_dl_dir = temp_base / str(run_id)
if run_dl_dir.exists():
shutil.rmtree(run_dl_dir)
run_dl_dir.mkdir(parents=True, exist_ok=True)
for artifact in rs_artifacts:
status_file = _download_artifact(token, repo, artifact, run_dl_dir)
if status_file is None:
continue
statuses = _parse_status_file(status_file)
all_statuses.extend(statuses)
rs_artifacts = [
a for a in artifacts
if a.get("name", "").startswith(_REVIEW_STATUS_ARTIFACT_PREFIX)
]
if not rs_artifacts:
# A re-run can leave several non-expired artifacts with the same name,
# each carrying the same source — dedupe by source so the comment
# doesn't render duplicate sections.
seen: set[str] = set()
deduped: list[dict] = []
for status in all_statuses:
src = status.get("source", "")
if src in seen:
continue
# Clean temp dir for this run's artifacts.
run_dl_dir = temp_base / rid
if run_dl_dir.exists():
shutil.rmtree(run_dl_dir)
run_dl_dir.mkdir(parents=True, exist_ok=True)
for artifact in rs_artifacts:
status_file = _download_artifact(token, repo, artifact, run_dl_dir)
if status_file is None:
continue
statuses = _parse_status_file(status_file)
all_statuses.extend(statuses)
return all_statuses
if src:
seen.add(src)
deduped.append(status)
return deduped
# ---------------------------------------------------------------------------
@@ -454,11 +460,6 @@ def build_comment_body(
)
def _merge_statuses(statuses: list[dict]) -> str:
"""Merge a list of status arrays into one JSON string."""
return json.dumps(statuses) if statuses else ""
def _commit_info_for_state(commit_info: str, pending: list[str]) -> str:
"""Use past tense in the final comment after every CI job completes."""
if pending:
@@ -484,7 +485,9 @@ def run(
) -> int:
"""Poll for job statuses and update the PR comment until all done.
Returns 0 always — comment posting is best-effort.
Returns 0 on success; 1 when all jobs completed but a dependency
failed (so ``gh run rerun --failed`` can pick it up). Comment posting
is best-effort.
"""
asm = _import_assembler()
start = time.time()
@@ -526,14 +529,15 @@ def run(
if gone_pending:
print(f" → {len(gone_pending)} job(s) disappeared from pending: {', '.join(gone_pending)}")
# Dynamically fetch all review-status artifacts from every run.
# Dynamically fetch all review-status artifacts from the run.
artifact_statuses = fetch_all_review_statuses(token, repo, run_id)
if len(artifact_statuses) != prev_artifact_count:
artifact_count_changed = len(artifact_statuses) != prev_artifact_count
if artifact_count_changed:
print(f" Found {len(artifact_statuses)} review status entries from artifacts "
f"(was {prev_artifact_count} last poll)")
prev_artifact_count = len(artifact_statuses)
merged_json = _merge_statuses(artifact_statuses)
merged_json = json.dumps(artifact_statuses) if artifact_statuses else ""
current_commit_info = _commit_info_for_state(commit_info, pending)
body = build_comment_body(
@@ -550,7 +554,7 @@ def run(
change_reasons.append(f"{len(new_pending)} new pending job(s)")
if gone_pending:
change_reasons.append(f"{len(gone_pending)} job(s) left pending")
if len(artifact_statuses) != prev_artifact_count:
if artifact_count_changed:
change_reasons.append("artifact statuses updated")
if not change_reasons:
change_reasons.append("initial post")

207
tests/ci/test_live_comment.py
View File

@@ -1,207 +0,0 @@
"""Tests for scripts/ci/live_comment.py — classify_jobs() + artifact helpers.
The poller's core logic is a pure function: take raw GitHub API job dicts
and split them into (completed, pending). Artifact parsing helpers are also
pure and tested here. The API wrapper + polling loop are tested via E2E
in CI, not here.
"""
from __future__ import annotations
import importlib.util
import json
import sys
import tempfile
from pathlib import Path
_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "live_comment.py"
_spec = importlib.util.spec_from_file_location("live_comment", _PATH)
if _spec is None or _spec.loader is None:
raise ImportError("Failed to load live_comment.py")
_mod = importlib.util.module_from_spec(_spec)
sys.modules["live_comment"] = _mod
_spec.loader.exec_module(_mod)
def _job(name: str, status: str, conclusion: str | None = None, workflow: str = "") -> dict:
"""Build a raw API job dict."""
j = {"name": name, "status": status, "conclusion": conclusion}
if workflow:
j["_workflow_name"] = workflow
return j
def test_classify_success():
jobs = [_job("Python tests", "completed", "success")]
completed, pending, job_urls = _mod.classify_jobs(jobs)
assert completed == {"Python tests": "success"}
assert pending == []
def test_classify_failure():
jobs = [_job("Python tests", "completed", "failure")]
completed, pending, job_urls = _mod.classify_jobs(jobs)
assert completed == {"Python tests": "failure"}
assert pending == []
def test_classify_queued():
jobs = [_job("Python tests", "queued", None)]
completed, pending, job_urls = _mod.classify_jobs(jobs)
assert completed == {}
assert pending == ["Python tests"]
def test_classify_mixed():
jobs = [
_job("Python tests", "completed", "success"),
_job("Python lints", "completed", "failure"),
_job("JS & TS checks", "in_progress", None),
_job("Desktop E2E", "queued", None),
]
completed, pending, job_urls = _mod.classify_jobs(jobs)
assert completed == {"Python tests": "success", "Python lints": "failure"}
assert set(pending) == {"JS & TS checks", "Desktop E2E"}
def test_classify_sub_workflow_jobs_prefixed():
"""Sub-workflow jobs get 'Workflow / job' display names."""
jobs = [
_job("test", "completed", "success", workflow="Tests"),
_job("check", "in_progress", None, workflow="JS Tests"),
]
completed, pending, job_urls = _mod.classify_jobs(jobs)
assert "Tests / test" in completed
assert completed["Tests / test"] == "success"
assert "JS Tests / check" in pending
def test_classify_captures_html_url():
"""The poller captures html_url per job for per-job log links."""
jobs = [
{**_job("Python tests", "completed", "failure"),
"html_url": "https://github.com/repo/actions/runs/1/job/2"},
_job("Python lints", "completed", "success"),
]
completed, pending, job_urls = _mod.classify_jobs(jobs)
assert job_urls["Python tests"] == "https://github.com/repo/actions/runs/1/job/2"
# Jobs without html_url are simply absent from the dict
assert "Python lints" not in job_urls
def test_classify_timed_out_treated_as_failure():
jobs = [_job("Python tests", "completed", "timed_out")]
completed, pending, job_urls = _mod.classify_jobs(jobs)
assert completed == {"Python tests": "failure"}
def test_commit_info_uses_present_tense_while_jobs_are_pending():
info = "<sub>running on [abc1234](https://commit-url) — fix: thing</sub>"
assert _mod._commit_info_for_state(info, ["Python tests"]) == info
def test_commit_info_uses_past_tense_after_jobs_complete():
info = "<sub>running on [abc1234](https://commit-url) — fix: thing</sub>"
assert _mod._commit_info_for_state(info, []) == (
"<sub>ran on [abc1234](https://commit-url) — fix: thing</sub>"
)
# ---------------------------------------------------------------------------
# Artifact parsing helpers
# ---------------------------------------------------------------------------
def test_parse_status_file_with_prefix():
"""GITHUB_OUTPUT format: review_status=<json>"""
with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f:
f.write('review_status=[{"source":"test","results":[]}]')
f.flush()
statuses = _mod._parse_status_file(Path(f.name))
assert len(statuses) == 1
assert statuses[0]["source"] == "test"
def test_parse_status_file_without_prefix():
"""Raw JSON (no review_status= prefix) is also accepted."""
with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f:
f.write('[{"source":"raw","results":[]}]')
f.flush()
statuses = _mod._parse_status_file(Path(f.name))
assert len(statuses) == 1
assert statuses[0]["source"] == "raw"
def test_parse_status_file_empty_array():
with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f:
f.write('review_status=[]')
f.flush()
statuses = _mod._parse_status_file(Path(f.name))
assert statuses == []
def test_parse_status_file_invalid_json():
with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f:
f.write('review_status=not json')
f.flush()
statuses = _mod._parse_status_file(Path(f.name))
assert statuses == []
def test_parse_status_file_nonexistent():
assert _mod._parse_status_file(Path("/nonexistent/file.json")) == []
def test_parse_status_file_not_a_list():
with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f:
f.write('review_status={"not":"a list"}')
f.flush()
statuses = _mod._parse_status_file(Path(f.name))
assert statuses == []
def test_merge_statuses_empty():
assert _mod._merge_statuses([]) == ""
def test_merge_statuses_single():
statuses = [{"source": "a", "results": []}]
result = _mod._merge_statuses(statuses)
assert json.loads(result) == statuses
def test_merge_statuses_multiple():
statuses = [
{"source": "a", "results": []},
{"source": "b", "results": [{"kind": "warning"}]},
]
result = _mod._merge_statuses(statuses)
assert json.loads(result) == statuses
def test_review_status_artifact_prefix():
"""The prefix is used to filter artifacts from the API."""
assert _mod._REVIEW_STATUS_ARTIFACT_PREFIX == "review-status-"
# Artifacts with this prefix should be picked up
assert "review-status-ci-timings".startswith(_mod._REVIEW_STATUS_ARTIFACT_PREFIX)
assert "review-status-review-labels".startswith(_mod._REVIEW_STATUS_ARTIFACT_PREFIX)
# Artifacts without it should not
assert not "ci-timings-report".startswith(_mod._REVIEW_STATUS_ARTIFACT_PREFIX)
assert not "playwright-report".startswith(_mod._REVIEW_STATUS_ARTIFACT_PREFIX)