hive-forge: name the CI job ci-log actually served
`ci-log --run <n> --job <idx>` 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.
This commit is contained in:
parent
c5b86afcb0
commit
cdac6091eb
2 changed files with 79 additions and 4 deletions
|
|
@ -10,7 +10,9 @@ use std::path::PathBuf;
|
||||||
use anyhow::{Context, Result, anyhow, bail};
|
use anyhow::{Context, Result, anyhow, bail};
|
||||||
use forgejo_api::{Auth, ForgejoError};
|
use forgejo_api::{Auth, ForgejoError};
|
||||||
use reqwest::blocking::{Client as HttpClient, Response};
|
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::{Deserialize, Serialize};
|
||||||
use serde_json::Value;
|
use serde_json::Value;
|
||||||
|
|
||||||
|
|
@ -218,10 +220,30 @@ impl Client {
|
||||||
/// attachment downloads. Uses the full URL as-is; the caller is
|
/// attachment downloads. Uses the full URL as-is; the caller is
|
||||||
/// responsible for constructing it (see `attachment_url`).
|
/// responsible for constructing it (see `attachment_url`).
|
||||||
pub fn get_bytes_raw(&self, url: &str) -> Result<Vec<u8>> {
|
pub fn get_bytes_raw(&self, url: &str) -> Result<Vec<u8>> {
|
||||||
|
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<u8>, Option<String>)> {
|
||||||
let resp = self.web.get(url).send().context("GET")?;
|
let resp = self.web.get(url).send().context("GET")?;
|
||||||
let resp = check_status(resp, &format!("GET {url}"))?;
|
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()
|
resp.bytes()
|
||||||
.map(|b| b.to_vec())
|
.map(|b| (b.to_vec(), name))
|
||||||
.with_context(|| format!("read bytes for GET {url}"))
|
.with_context(|| format!("read bytes for GET {url}"))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -449,6 +471,18 @@ fn read_token() -> Result<String> {
|
||||||
Ok(raw.trim().to_owned())
|
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<String> {
|
||||||
|
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
|
/// Surface non-2xx HTTP responses on the raw web routes as anyhow
|
||||||
/// errors with the response body included (matches `curl
|
/// errors with the response body included (matches `curl
|
||||||
/// --fail-with-body`) — turns silent failures into errors with a
|
/// --fail-with-body`) — turns silent failures into errors with a
|
||||||
|
|
@ -464,7 +498,35 @@ fn check_status(resp: Response, op: &str) -> Result<Response> {
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
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]
|
#[test]
|
||||||
fn owner_repo_from_git_url_strips_scheme_and_creds() {
|
fn owner_repo_from_git_url_strips_scheme_and_creds() {
|
||||||
|
|
|
||||||
|
|
@ -172,7 +172,7 @@ fn persisted_logs(client: &Client, repo: &str, args: &Args) -> Result<bool> {
|
||||||
"/{repo}/actions/runs/{}/jobs/{}/attempt/{}/logs",
|
"/{repo}/actions/runs/{}/jobs/{}/attempt/{}/logs",
|
||||||
args.run, args.job, args.attempt
|
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);
|
return Ok(false);
|
||||||
};
|
};
|
||||||
if bytes.is_empty() {
|
if bytes.is_empty() {
|
||||||
|
|
@ -184,10 +184,18 @@ fn persisted_logs(client: &Client, repo: &str, args: &Args) -> Result<bool> {
|
||||||
"run": args.run,
|
"run": args.run,
|
||||||
"job": args.job,
|
"job": args.job,
|
||||||
"source": "persisted",
|
"source": "persisted",
|
||||||
|
"served": served,
|
||||||
"log": text,
|
"log": text,
|
||||||
}))?;
|
}))?;
|
||||||
return Ok(true);
|
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}");
|
print!("{text}");
|
||||||
if !text.ends_with('\n') {
|
if !text.ends_with('\n') {
|
||||||
println!();
|
println!();
|
||||||
|
|
@ -200,6 +208,11 @@ fn persisted_logs(client: &Client, repo: &str, args: &Args) -> Result<bool> {
|
||||||
/// Returns an error if the run/job can't be fetched from either source
|
/// 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`
|
/// (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.
|
/// 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<()> {
|
pub fn run(client: &Client, args: Args) -> Result<()> {
|
||||||
let repo = client.repo()?;
|
let repo = client.repo()?;
|
||||||
let stream_path = format!("/{repo}/actions/runs/{}/jobs/{}", args.run, args.job);
|
let stream_path = format!("/{repo}/actions/runs/{}/jobs/{}", args.run, args.job);
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue