From cdac6091eb2983dc8305dae42363c3c490e66b63 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 2 Sep 2026 17:39:09 +0200 Subject: [PATCH] hive-forge: name the CI job ci-log actually served `ci-log --run --job ` accepted any index and exited 0. Past the run's job count it printed job 0's log, with nothing marking the substitution. Measured against run 3363 (4 jobs): indices 0-3 gave three distinct md5s, while 99 and 12345 both returned output byte-identical to job 0. Both web log routes clamp an out-of-range index to job 0 and answer 200. I had claimed nothing in the response distinguishes them -- that was measured on the body only, and the body was the wrong place to look. The `Content-Disposition` header names the job the server actually served: --job 3 -> filename="ci-doc-pointer lint-8363.log" --job 99 -> filename="ci-nix flake check-8360.log" (job 0's) So the fix is to report what came back rather than to pre-validate the index. Establishing the real job count needs two extra API calls on every indexed read -- `ActionRun` carries no job count, and the only job endpoints are the per-run listing and a per-job log keyed by internal job id, not by index. The header costs nothing: it is already in the response being read. The provenance line goes to stderr, so it cannot corrupt a piped log; under `--json` it is a `served` field instead. An out-of-range `--job` is therefore no longer an error -- it is a read whose true subject is named. `get_bytes_named` is a sibling of `get_bytes_raw` rather than a signature change, leaving the attachment and artifact download paths untouched. Rebased onto main after the two flat-rename PRs landed; the only conflict was the test module's import list, resolved by keeping both sides. While reading the surrounding context this commit's own defect surfaced: `check_status`'s doc comment had been left glued to the head of `disposition_filename`'s, so one function carried two unrelated descriptions and the other carried none. No gate can see that -- it is well-formed rustdoc either way. --- hive-forge/src/client.rs | 68 ++++++++++++++++++++++++++++++++-- hive-forge/src/verbs/ci_log.rs | 15 +++++++- 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/hive-forge/src/client.rs b/hive-forge/src/client.rs index 11670b48..9b91aae5 100644 --- a/hive-forge/src/client.rs +++ b/hive-forge/src/client.rs @@ -10,7 +10,9 @@ use std::path::PathBuf; use anyhow::{Context, Result, anyhow, bail}; use forgejo_api::{Auth, ForgejoError}; use reqwest::blocking::{Client as HttpClient, Response}; -use reqwest::header::{ACCEPT, AUTHORIZATION, CONTENT_TYPE, HeaderMap, HeaderValue}; +use reqwest::header::{ + ACCEPT, AUTHORIZATION, CONTENT_DISPOSITION, CONTENT_TYPE, HeaderMap, HeaderValue, +}; use serde::{Deserialize, Serialize}; use serde_json::Value; @@ -218,10 +220,30 @@ impl Client { /// attachment downloads. Uses the full URL as-is; the caller is /// responsible for constructing it (see `attachment_url`). pub fn get_bytes_raw(&self, url: &str) -> Result> { + self.get_bytes_named(url).map(|(bytes, _)| bytes) + } + + /// Like [`Client::get_bytes_raw`], but also returns the filename the + /// server put in `Content-Disposition`. + /// + /// Forgejo's persisted Actions-log route names the download after the + /// job it actually served, and that header is the **only** part of the + /// response identifying which job came back: the route clamps an + /// out-of-range job index to job 0 and answers 200, so the body alone + /// is byte-identical to a legitimate read. + /// + /// # Errors + /// Same as [`Client::get_bytes_raw`] — transport failure or a non-2xx. + pub fn get_bytes_named(&self, url: &str) -> Result<(Vec, Option)> { let resp = self.web.get(url).send().context("GET")?; let resp = check_status(resp, &format!("GET {url}"))?; + let name = resp + .headers() + .get(CONTENT_DISPOSITION) + .and_then(|v| v.to_str().ok()) + .and_then(disposition_filename); resp.bytes() - .map(|b| b.to_vec()) + .map(|b| (b.to_vec(), name)) .with_context(|| format!("read bytes for GET {url}")) } @@ -449,6 +471,18 @@ fn read_token() -> Result { Ok(raw.trim().to_owned()) } +/// The quoted `filename="…"` out of a `Content-Disposition` value. +/// +/// Deliberately reads the plain `filename=` parameter and not the RFC 5987 +/// `filename*=UTF-8''…` one that Forgejo sends alongside it: the starred +/// form is percent-encoded, so it needs decoding before it can be shown, +/// and the plain form carries the same name. +fn disposition_filename(value: &str) -> Option { + let after = value.split_once("filename=\"")?.1; + let (name, _) = after.split_once('"')?; + (!name.is_empty()).then(|| name.to_owned()) +} + /// Surface non-2xx HTTP responses on the raw web routes as anyhow /// errors with the response body included (matches `curl /// --fail-with-body`) — turns silent failures into errors with a @@ -464,7 +498,35 @@ fn check_status(resp: Response, op: &str) -> Result { #[cfg(test)] mod tests { - use super::{index, owner_repo_from_git_url, split_repo}; + use super::{disposition_filename, index, owner_repo_from_git_url, split_repo}; + + #[test] + fn disposition_filename_reads_forgejos_real_header() { + // Verbatim from a persisted Actions-log download, both parameters + // present in the order Forgejo emits them. + let v = "attachment; filename=\"ci-doc-pointer lint-8363.log\"; \ + filename*=UTF-8''ci-doc-pointer%20lint-8363.log"; + assert_eq!( + disposition_filename(v).as_deref(), + Some("ci-doc-pointer lint-8363.log") + ); + } + + #[test] + fn disposition_filename_absent_or_unusable() { + assert_eq!(disposition_filename("attachment"), None); + assert_eq!(disposition_filename(""), None); + // Only the starred form: not decoded here, so it must not be + // mistaken for a usable plain filename. + assert_eq!( + disposition_filename("attachment; filename*=UTF-8''a%20b.log"), + None + ); + // Unterminated quote — must not return a truncated name. + assert_eq!(disposition_filename("attachment; filename=\"oops"), None); + // Present but empty. + assert_eq!(disposition_filename("attachment; filename=\"\""), None); + } #[test] fn owner_repo_from_git_url_strips_scheme_and_creds() { diff --git a/hive-forge/src/verbs/ci_log.rs b/hive-forge/src/verbs/ci_log.rs index a918d6ce..d9037d61 100644 --- a/hive-forge/src/verbs/ci_log.rs +++ b/hive-forge/src/verbs/ci_log.rs @@ -172,7 +172,7 @@ fn persisted_logs(client: &Client, repo: &str, args: &Args) -> Result { "/{repo}/actions/runs/{}/jobs/{}/attempt/{}/logs", args.run, args.job, args.attempt )); - let Ok(bytes) = client.get_bytes_raw(&url) else { + let Ok((bytes, served)) = client.get_bytes_named(&url) else { return Ok(false); }; if bytes.is_empty() { @@ -184,10 +184,18 @@ fn persisted_logs(client: &Client, repo: &str, args: &Args) -> Result { "run": args.run, "job": args.job, "source": "persisted", + "served": served, "log": text, }))?; return Ok(true); } + // Which job the server actually served, on stderr so it can't corrupt + // a piped log. This route clamps an out-of-range `--job` to job 0 and + // answers 200, so without naming the file back there is no way to tell + // a substituted log from the one that was asked for. + if let Some(name) = &served { + eprintln!("hive-forge: run #{} job {} -> {name}", args.run, args.job); + } print!("{text}"); if !text.ends_with('\n') { println!(); @@ -200,6 +208,11 @@ fn persisted_logs(client: &Client, repo: &str, args: &Args) -> Result { /// Returns an error if the run/job can't be fetched from either source /// (network, an unknown run number, or a non-2xx), if a requested `--step` /// is out of range, or if both sources yield no log content. +/// +/// An out-of-range `--job` is **not** an error: the web routes clamp it to +/// job 0 and answer 200, and establishing the real job count costs an extra +/// two API calls on every indexed read. [`persisted_logs`] names the job the +/// server actually served instead, from a header already in the response. pub fn run(client: &Client, args: Args) -> Result<()> { let repo = client.repo()?; let stream_path = format!("/{repo}/actions/runs/{}/jobs/{}", args.run, args.job);