diff --git a/crates/sylpheed-formats/examples/default_owners.rs b/crates/sylpheed-formats/examples/default_owners.rs index c1f64db2..820b6882 100644 --- a/crates/sylpheed-formats/examples/default_owners.rs +++ b/crates/sylpheed-formats/examples/default_owners.rs @@ -25,8 +25,12 @@ fn is_value(s: &str) -> bool { } fn main() { - let root = std::env::var("SYLPHEED_DISC") - .unwrap_or_else(|_| "/home/fabi/RE - Project Sylpheed/sylph_extract".into()); + // No hardcoded fallback: it made SYLPHEED_DISC look like a control while + // one machine's directory layout decided the outcome (#16). + let Ok(root) = std::env::var("SYLPHEED_DISC") else { + eprintln!("set SYLPHEED_DISC to the extracted disc root"); + std::process::exit(2); + }; let wanted: Vec = std::env::args().skip(1).collect(); let arc = PakArchive::open(std::path::Path::new(&root).join("dat/GP_MAIN_GAME_E.pak")).unwrap(); diff --git a/crates/sylpheed-formats/examples/defaulted_fields.rs b/crates/sylpheed-formats/examples/defaulted_fields.rs index 63786351..d2bc1952 100644 --- a/crates/sylpheed-formats/examples/defaulted_fields.rs +++ b/crates/sylpheed-formats/examples/defaulted_fields.rs @@ -31,9 +31,14 @@ fn is_value(s: &str) -> bool { } fn main() { + // argv[1], else SYLPHEED_DISC. No hardcoded fallback -- see #16. let root = std::env::args() .nth(1) - .unwrap_or_else(|| "/home/fabi/RE - Project Sylpheed/sylph_extract".into()); + .or_else(|| std::env::var("SYLPHEED_DISC").ok()) + .unwrap_or_else(|| { + eprintln!("usage: defaulted_fields [paks...] (or set SYLPHEED_DISC)"); + std::process::exit(2); + }); let paks: Vec = std::env::args().skip(2).collect(); let paks = if paks.is_empty() { vec![ diff --git a/crates/sylpheed-formats/tests/caption_families_disc.rs b/crates/sylpheed-formats/tests/caption_families_disc.rs index f344c998..d3f4f199 100644 --- a/crates/sylpheed-formats/tests/caption_families_disc.rs +++ b/crates/sylpheed-formats/tests/caption_families_disc.rs @@ -4,31 +4,11 @@ //! `build_caption_text` generalises the key parser to all eight. use std::collections::BTreeMap; -use std::path::{Path, PathBuf}; use sylpheed_formats::{movie_subtitle, PakArchive}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let d = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - d.join("dat").is_dir().then(|| d.to_path_buf()) -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: set SYLPHEED_DISC"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; #[test] fn all_eight_caption_families_are_read() { diff --git a/crates/sylpheed-formats/tests/common/mod.rs b/crates/sylpheed-formats/tests/common/mod.rs new file mode 100644 index 00000000..649e016f --- /dev/null +++ b/crates/sylpheed-formats/tests/common/mod.rs @@ -0,0 +1,79 @@ +//! One place that decides where the disc corpora are — issue #16, remedy (3). +//! +//! # What this replaces +//! +//! Seventeen files under `tests/` each defined their own `disc_root()`, and they +//! had **already drifted into five variants**. Four were the same thing written +//! four ways (differing only in return type and style). The fifth — +//! `movie_manifest_disc`, `movie_subtitle_disc`, `slb_disc` — did something +//! materially different: it honoured `SYLPHEED_DISC` **and nothing else**. +//! +//! So one function name meant two different things in one directory, which is +//! the same "one name, several meanings" defect #16 identifies in +//! `SYLPHEED_DISC` itself and in `#[ignore]`. +//! +//! # Why the env var, and no fallback +//! +//! The fourteen copies with a fallback hardcoded one machine's absolute layout: +//! +//! ```text +//! /home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja) +//! ``` +//! +//! That made `unset SYLPHEED_DISC` a no-op there: whether the disc suites ran +//! was a property of *the machine's directory layout*, invisible in the command +//! and in the output. The env var looked like a control and was not one. +//! +//! This module adopts the behaviour three of those files already had, rather +//! than inventing a new one: **the environment decides, always.** Point +//! `SYLPHEED_DISC` at the extracted disc and the suites run; leave it unset and +//! they skip. Same command, same answer, on every machine. +//! +//! `just test-disc` reads `.env` (already gitignored as a local dev override) +//! so no absolute path has to live in the source tree again. +//! +//! # This does not fix the tally +//! +//! A skipped suite still counts as `passed` — `#[ignore]` is static and cannot +//! move at runtime. That is why `tests/corpus_report.rs` exists: it prints which +//! corpora resolved, and it is the thing to read. This module only makes the +//! *control* honest, so that report can now say `PRESENT via $SYLPHEED_DISC` +//! and mean it. +// `tests/common/mod.rs` is compiled into EVERY integration-test binary, and each +// one uses only the resolver (and maybe the macro) it needs. Without these, every +// binary warns about the parts it did not use. +#![allow(dead_code, unused_macros, unused_imports)] + +use std::path::PathBuf; + +/// The extracted disc root — the directory containing `dat/`. +pub fn disc_root() -> Option { + let p = PathBuf::from(std::env::var("SYLPHEED_DISC").ok()?); + p.join("dat").is_dir().then_some(p) +} + +/// The extracted `resource3d` directory (`Stage_SNN.xpr` models). +pub fn res3d_dir() -> Option { + let p = PathBuf::from(std::env::var("SYLPHEED_RES3D").ok()?); + p.is_dir().then_some(p) +} + +/// The retail ISO image itself, not a directory. +pub fn iso_path() -> Option { + let p = PathBuf::from(std::env::var("SYLPHEED_ISO").ok()?); + p.is_file().then_some(p) +} + +/// Bind the disc root or return from the test. +/// +/// The early return keeps the test *passing*, which is why the tally cannot +/// distinguish a skip from a real run — see `corpus_report.rs`. +macro_rules! skip_without_disc { + ($root:ident) => { + let Some($root) = crate::common::disc_root() else { + eprintln!("SKIP: set SYLPHEED_DISC to the extracted disc root"); + return; + }; + }; +} +pub(crate) use skip_without_disc; diff --git a/crates/sylpheed-formats/tests/corpus_report.rs b/crates/sylpheed-formats/tests/corpus_report.rs index cc6221f6..e69da0b2 100644 --- a/crates/sylpheed-formats/tests/corpus_report.rs +++ b/crates/sylpheed-formats/tests/corpus_report.rs @@ -3,94 +3,62 @@ //! # Why this exists //! //! `cargo test --workspace` reports **the same tally whether or not the disc -//! corpus was exercised** — see issue #16. Measured: the disc suites ran on a -//! developer desktop (1 936 s, `mesh_consistency_disc` alone 1 220 s) and -//! skipped on CI (2.4 s total), and *both* reported `207 passed / 0 failed / -//! 14 ignored` across 30 suites. +//! corpus was exercised** — issue #16. Measured on `main` @ `4ca0b8e`: this +//! desktop ran the disc suites (`mesh_consistency_disc` alone **1 215 s**) and +//! CI skipped them (2.4 s total), and *both* reported +//! `209 passed / 0 failed / 14 ignored` across 31 suites. //! -//! Two mechanisms compound, and either alone would be survivable: +//! Two mechanisms compound: //! -//! 1. **A skip is a passing test.** The disc suites do `eprintln!("SKIP: …")` -//! and return early from a test that still passes, so a skipped suite and a -//! fully exercised one both score `1 passed`. The totals are invariant. -//! 2. **The message is invisible.** `cargo test` captures a *passing* test's -//! output, so neither log contains a `SKIP:` line. The absence of one proves -//! nothing, which makes the obvious check useless too. +//! 1. **A skip is a passing test.** The gated suites print `SKIP:` and return +//! early from a test that still passes, so a skipped suite and a fully +//! exercised one both score `1 passed`. +//! 2. **The message is invisible.** `cargo test` captures a passing test's +//! output, so neither log contains a `SKIP:` line. //! //! And `14 ignored` cannot help: `#[ignore]` is static, so that column is the //! literal count of attributes in the source and cannot move at runtime. //! -//! This test is the fix for the *report*, not for the control. It always runs, -//! never fails, and records what was actually available — to a file, because a -//! passing test's stdout is captured and would be invisible in exactly the CI -//! log that needs it. +//! This test fixes the *report*. `tests/common/mod.rs` fixes the *control*. use std::fmt::Write as _; use std::io::Write as _; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; -/// Resolution mirrors the per-suite helpers exactly. If one of those changes, -/// this drifts — which is itself an argument for the shared helper in #16's -/// remedy (3). -fn resolve( - env: &str, - fallback: &str, - want_dir_child: Option<&str>, - want_file: bool, -) -> (String, Option) { - let ok = |p: &Path| -> bool { - match (want_dir_child, want_file) { - (Some(child), _) => p.join(child).is_dir(), - (None, true) => p.is_file(), - (None, false) => p.is_dir(), - } - }; - if let Ok(v) = std::env::var(env) { - let p = PathBuf::from(&v); - if ok(&p) { - return (format!("PRESENT via ${env}"), Some(p)); - } - return (format!("${env} is set but does not resolve: {v}"), None); - } - let p = PathBuf::from(fallback); - if ok(&p) { - // The important case. ${env} is unset, yet the corpus resolved anyway, - // so the suites run because of this machine's directory layout — a - // property invisible in the command and in the output. - return ( - format!("PRESENT via the HARDCODED fallback, NOT ${env}"), - Some(p), - ); - } - ( - "ABSENT — the gated suites will self-skip and still count as passed".into(), - None, - ) -} +mod common; -fn target_dir() -> Option { - let exe = std::env::current_exe().ok()?; - exe.ancestors() - .find(|a| a.file_name().is_some_and(|n| n == "target")) - .map(PathBuf::from) +/// Describe one corpus. Resolution is delegated to `common`, so this cannot +/// drift from what the suites themselves do — the previous version duplicated +/// the resolution logic and said so in its own comments. +fn status(var: &str, resolved: Option) -> (String, Option) { + match (std::env::var(var), resolved) { + (Ok(_), Some(p)) => (format!("PRESENT via ${var}"), Some(p)), + // Set but unusable is NOT the same as absent, and wants a different + // fix: a typo or a moved directory rather than a machine without the + // corpus. Reporting them alike would send someone hunting the wrong one. + (Ok(v), None) => (format!("${var} is set but does not resolve: {v}"), None), + (Err(_), _) => ( + format!("ABSENT — ${var} unset; its suites self-skip and still count as passed"), + None, + ), + } } #[test] fn corpus_report() { - let corpora = [ - ("SYLPHEED_DISC", "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", Some("dat"), false), - ("SYLPHEED_RES3D", "/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d", None, false), - ("SYLPHEED_ISO", "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja).iso", None, true), + let rows = [ + status("SYLPHEED_DISC", common::disc_root()), + status("SYLPHEED_RES3D", common::res3d_dir()), + status("SYLPHEED_ISO", common::iso_path()), ]; let mut out = String::from("test corpus report (issue #16)\n"); let mut any = false; - for (env, fallback, child, file) in corpora { - let (status, path) = resolve(env, fallback, child, file); + for (text, path) in &rows { any |= path.is_some(); - let _ = writeln!(out, " {env:<15} {status}"); + let _ = writeln!(out, " {text}"); if let Some(p) = path { - let _ = writeln!(out, " {:<15} -> {}", "", p.display()); + let _ = writeln!(out, " -> {}", p.display()); } } let _ = writeln!( @@ -107,15 +75,14 @@ fn corpus_report() { ); } - // Captured for a passing test, so it is only visible with --show-output or - // --nocapture. Kept anyway: it is the natural place to look locally. + // Captured for a passing test, so visible only with --show-output. Kept + // because it is the natural place to look locally. println!("{out}"); // The channel that survives capture, and the one CI reads. if let Some(dir) = target_dir() { let _ = std::fs::write(dir.join("sylpheed-corpus-report.txt"), &out); } - // Gitea and GitHub both honour this; it puts the block in the run summary. if let Ok(p) = std::env::var("GITHUB_STEP_SUMMARY") { if let Ok(mut f) = std::fs::OpenOptions::new() .create(true) @@ -127,31 +94,30 @@ fn corpus_report() { } } -/// The ABSENT branch is the one CI takes, and it cannot be reached on a machine -/// that has the corpora — so it is exercised directly here rather than shipped -/// unrun. Same for the "set but does not resolve" branch, which is what a typo -/// in the env var produces. -#[test] -fn resolve_renders_every_branch() { - let missing = "/nonexistent/sylpheed/corpus"; - - // env unset + fallback missing -> ABSENT - std::env::remove_var("SYLPHEED_TEST_PROBE"); - let (status, path) = resolve("SYLPHEED_TEST_PROBE", missing, None, false); - assert!(status.starts_with("ABSENT"), "{status}"); - assert!(path.is_none()); - - // env set to something that does not resolve -> named as such, NOT absent, - // because those two states want different fixes. - std::env::set_var("SYLPHEED_TEST_PROBE", missing); - let (status, path) = resolve("SYLPHEED_TEST_PROBE", missing, None, false); - assert!(status.contains("is set but does not resolve"), "{status}"); - assert!(path.is_none()); - - // env set and valid -> attributed to the env var, not the fallback - std::env::set_var("SYLPHEED_TEST_PROBE", env!("CARGO_MANIFEST_DIR")); - let (status, path) = resolve("SYLPHEED_TEST_PROBE", missing, None, false); - assert_eq!(status, "PRESENT via $SYLPHEED_TEST_PROBE"); - assert!(path.is_some()); - std::env::remove_var("SYLPHEED_TEST_PROBE"); +fn target_dir() -> Option { + let exe = std::env::current_exe().ok()?; + exe.ancestors() + .find(|a| a.file_name().is_some_and(|n| n == "target")) + .map(PathBuf::from) +} + +/// The three states must render distinctly, and CI only ever exercises one of +/// them — so they are driven directly here rather than shipped unrun. +#[test] +fn status_renders_every_state() { + let probe = "SYLPHEED_TEST_PROBE"; + std::env::remove_var(probe); + let (text, path) = status(probe, None); + assert!(text.starts_with("ABSENT"), "{text}"); + assert!(path.is_none()); + + std::env::set_var(probe, "/nonexistent/sylpheed/corpus"); + let (text, _) = status(probe, None); + assert!(text.contains("is set but does not resolve"), "{text}"); + + std::env::set_var(probe, env!("CARGO_MANIFEST_DIR")); + let (text, path) = status(probe, Some(PathBuf::from(env!("CARGO_MANIFEST_DIR")))); + assert_eq!(text, format!("PRESENT via ${probe}")); + assert!(path.is_some()); + std::env::remove_var(probe); } diff --git a/crates/sylpheed-formats/tests/idxd_records_disc.rs b/crates/sylpheed-formats/tests/idxd_records_disc.rs index 3c84e01d..48bf42d8 100644 --- a/crates/sylpheed-formats/tests/idxd_records_disc.rs +++ b/crates/sylpheed-formats/tests/idxd_records_disc.rs @@ -13,27 +13,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::hash::tag_hash; use sylpheed_formats::{IdxdObject, PakArchive}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - default.join("dat").is_dir().then(|| default.to_path_buf()) -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; /// Every `.pak` on the disc, recursively. /// diff --git a/crates/sylpheed-formats/tests/ixud_records_disc.rs b/crates/sylpheed-formats/tests/ixud_records_disc.rs index c833ad57..bd465dc4 100644 --- a/crates/sylpheed-formats/tests/ixud_records_disc.rs +++ b/crates/sylpheed-formats/tests/ixud_records_disc.rs @@ -8,27 +8,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::hash::ixud_hash_str; use sylpheed_formats::{IxudObject, PakArchive}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let d = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - d.join("dat").is_dir().then(|| d.to_path_buf()) -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: set SYLPHEED_DISC"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; fn all_paks(root: &Path) -> Vec { let mut out = Vec::new(); diff --git a/crates/sylpheed-formats/tests/mesh_consistency_disc.rs b/crates/sylpheed-formats/tests/mesh_consistency_disc.rs index 29392117..14ac2c36 100644 --- a/crates/sylpheed-formats/tests/mesh_consistency_disc.rs +++ b/crates/sylpheed-formats/tests/mesh_consistency_disc.rs @@ -11,28 +11,15 @@ //! rather than as a snapshot of the bug. use std::collections::BTreeMap; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use sylpheed_formats::mesh::Xbg7Model; /// Every place one model name was seen: (span, verts, tris, container). type Sightings = BTreeMap>; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; /// Rounded (w, h, d) of a model's own geometry. fn span(m: &Xbg7Model) -> Option<[i64; 3]> { diff --git a/crates/sylpheed-formats/tests/mesh_disc.rs b/crates/sylpheed-formats/tests/mesh_disc.rs index f6b912df..eba6eaf6 100644 --- a/crates/sylpheed-formats/tests/mesh_disc.rs +++ b/crates/sylpheed-formats/tests/mesh_disc.rs @@ -9,16 +9,8 @@ use std::path::PathBuf; use sylpheed_formats::mesh::{material_groups, node_transforms, submesh_albedos, Xbg7Model}; -fn res3d_dir() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_RES3D") { - let p = PathBuf::from(p); - if p.is_dir() { - return Some(p); - } - } - let default = PathBuf::from("/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d"); - default.is_dir().then_some(default) -} +mod common; +use common::res3d_dir; #[test] #[ignore = "requires extracted disc models — set SYLPHEED_RES3D"] @@ -378,10 +370,11 @@ fn hero_ship_grouped_pool_decodes() { #[ignore] fn stage_models_decode() { use sylpheed_formats::mesh::Xbg7Model; - let dir = std::env::var("SYLPHEED_RES3D").unwrap_or_else(|_| { - "/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d".to_string() - }); - let path = format!("{dir}/Stage_S10.xpr"); + let Some(dir) = res3d_dir() else { + eprintln!("SKIP: set SYLPHEED_RES3D to the extracted resource3d directory"); + return; + }; + let path = format!("{}/Stage_S10.xpr", dir.display()); let bytes = std::fs::read(&path).expect("read Stage_S10"); let models = Xbg7Model::stage_models(&bytes); for m in &models { @@ -423,9 +416,10 @@ fn stage_models_decode() { fn stage_models_sweep() { use std::time::Instant; use sylpheed_formats::mesh::Xbg7Model; - let dir = std::env::var("SYLPHEED_RES3D").unwrap_or_else(|_| { - "/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d".to_string() - }); + let Some(dir) = res3d_dir() else { + eprintln!("SKIP: set SYLPHEED_RES3D to the extracted resource3d directory"); + return; + }; let mut names: Vec<_> = std::fs::read_dir(&dir) .unwrap() .filter_map(|e| e.ok().map(|e| e.file_name().into_string().unwrap())) @@ -434,7 +428,7 @@ fn stage_models_sweep() { names.sort(); let mut tot = 0usize; for n in &names { - let bytes = std::fs::read(format!("{dir}/{n}")).unwrap(); + let bytes = std::fs::read(format!("{}/{n}", dir.display())).unwrap(); let t0 = Instant::now(); let models = Xbg7Model::stage_models(&bytes); let dt = t0.elapsed().as_millis(); @@ -455,10 +449,11 @@ fn stage_models_sweep() { #[ignore] fn stage_models_quality_audit() { use sylpheed_formats::mesh::Xbg7Model; - let dir = std::env::var("SYLPHEED_RES3D").unwrap_or_else(|_| { - "/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d".to_string() - }); - let bytes = std::fs::read(format!("{dir}/Stage_S07.xpr")).unwrap(); + let Some(dir) = res3d_dir() else { + eprintln!("SKIP: set SYLPHEED_RES3D to the extracted resource3d directory"); + return; + }; + let bytes = std::fs::read(format!("{}/Stage_S07.xpr", dir.display())).unwrap(); let models = Xbg7Model::stage_models(&bytes); let (mut small, mut mid, mut huge, mut dupnames) = (0, 0, 0, 0); let mut seen = std::collections::HashSet::new(); diff --git a/crates/sylpheed-formats/tests/movie_manifest_disc.rs b/crates/sylpheed-formats/tests/movie_manifest_disc.rs index 9e253eec..73b90e02 100644 --- a/crates/sylpheed-formats/tests/movie_manifest_disc.rs +++ b/crates/sylpheed-formats/tests/movie_manifest_disc.rs @@ -7,10 +7,8 @@ use sylpheed_formats::movie_manifest; use sylpheed_formats::slb::VoiceLang; use sylpheed_formats::PakArchive; -fn disc_root() -> Option { - let p = PathBuf::from(std::env::var("SYLPHEED_DISC").ok()?); - p.join("dat").is_dir().then_some(p) -} +mod common; +use common::disc_root; /// Read the manifest + `eng\sounds.tbl` out of `tables.pak`. fn load_manifest_and_sounds(root: &Path) -> (Vec, Vec) { diff --git a/crates/sylpheed-formats/tests/movie_subtitle_disc.rs b/crates/sylpheed-formats/tests/movie_subtitle_disc.rs index 760874f5..6b0640c6 100644 --- a/crates/sylpheed-formats/tests/movie_subtitle_disc.rs +++ b/crates/sylpheed-formats/tests/movie_subtitle_disc.rs @@ -1,15 +1,11 @@ //! Real-disc test for the movie subtitle chain. Skipped when the extracted disc //! is absent (set `SYLPHEED_DISC` to the extract root to enable). -use std::path::PathBuf; - use sylpheed_formats::movie_subtitle::{self, SubLang}; use sylpheed_formats::PakArchive; -fn disc_root() -> Option { - let p = PathBuf::from(std::env::var("SYLPHEED_DISC").ok()?); - p.join("dat").is_dir().then_some(p) -} +mod common; +use common::disc_root; #[test] fn resolves_english_radio_subtitles() { diff --git a/crates/sylpheed-formats/tests/pak_idxd_disc.rs b/crates/sylpheed-formats/tests/pak_idxd_disc.rs index b7e14d61..d433d51f 100644 --- a/crates/sylpheed-formats/tests/pak_idxd_disc.rs +++ b/crates/sylpheed-formats/tests/pak_idxd_disc.rs @@ -1,40 +1,16 @@ //! Integration tests against the real extracted Project Sylpheed disc. //! //! These are **skipped** (pass as no-ops) when the extracted disc is not present, -//! so the suite still runs on machines/CI without the game. Point at the disc via -//! the `SYLPHEED_DISC` env var, or drop it at the default dev path below. - -use std::path::{Path, PathBuf}; +//! so the suite still runs on machines/CI without the game. Point at the disc +//! with the `SYLPHEED_DISC` env var; there is no path fallback (see #16). use sylpheed_formats::texture::X360Texture; use sylpheed_formats::{IdxdObject, PakArchive}; +mod common; +use common::skip_without_disc; + /// Locate the extracted disc root, or `None` to skip. -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); - return; - }; - }; -} - #[test] fn deftables_header_and_first_entry() { skip_without_disc!(root); diff --git a/crates/sylpheed-formats/tests/slb_disc.rs b/crates/sylpheed-formats/tests/slb_disc.rs index c9ac6168..2a6964aa 100644 --- a/crates/sylpheed-formats/tests/slb_disc.rs +++ b/crates/sylpheed-formats/tests/slb_disc.rs @@ -9,10 +9,8 @@ use sylpheed_formats::hash::name_hash; use sylpheed_formats::slb::{self, VoiceLang}; use sylpheed_formats::PakArchive; -fn disc_root() -> Option { - let p = PathBuf::from(std::env::var("SYLPHEED_DISC").ok()?); - p.join("dat").is_dir().then_some(p) -} +mod common; +use common::disc_root; /// Read `[off, off+size)` from `dat/sound.p00..` (segments concatenated). fn read_range(root: &Path, mut off: u64, size: usize) -> Vec { diff --git a/crates/sylpheed-formats/tests/slb_leading_segment_disc.rs b/crates/sylpheed-formats/tests/slb_leading_segment_disc.rs index 63d4f097..213a38ef 100644 --- a/crates/sylpheed-formats/tests/slb_leading_segment_disc.rs +++ b/crates/sylpheed-formats/tests/slb_leading_segment_disc.rs @@ -5,31 +5,12 @@ //! `RIFF`. The banks that looked fine were the ones whose leading segment is //! silence. One rule, two outcomes. -use std::path::{Path, PathBuf}; +use std::path::Path; use sylpheed_formats::{slb, PakArchive}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - default.join("dat").is_dir().then(|| default.to_path_buf()) -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: set SYLPHEED_DISC"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; fn bank(root: &Path, n: u32) -> Vec { let snd = PakArchive::open(root.join("dat/sound.pak")).expect("sound.pak"); diff --git a/crates/sylpheed-formats/tests/texture_disc.rs b/crates/sylpheed-formats/tests/texture_disc.rs index d3edf7fc..fcb1cbc7 100644 --- a/crates/sylpheed-formats/tests/texture_disc.rs +++ b/crates/sylpheed-formats/tests/texture_disc.rs @@ -1,28 +1,15 @@ //! Integration test: run the XPR2 texture pipeline against REAL `.xpr` files //! read directly from the retail disc image, reproducing exactly what the //! viewer's texture-preview path does (`identify_format` → `X360Texture:: -//! from_xpr2`). Skipped unless `SYLPHEED_ISO` points at the disc (or the -//! default dev path exists). +//! from_xpr2`). Skipped unless `SYLPHEED_ISO` points at the disc image. //! //! Run: `cargo test -p sylpheed-formats --test texture_disc -- --ignored --nocapture` -use std::path::PathBuf; - use sylpheed_formats::texture::X360Texture; use sylpheed_formats::vfs::identify_format; -fn iso_path() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_ISO") { - let p = PathBuf::from(p); - if p.is_file() { - return Some(p); - } - } - let default = PathBuf::from( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja).iso", - ); - default.is_file().then_some(default) -} +mod common; +use common::iso_path; #[tokio::test] #[ignore = "requires the retail ISO — set SYLPHEED_ISO"] diff --git a/crates/sylpheed-formats/tests/ui_focus_kind_disc.rs b/crates/sylpheed-formats/tests/ui_focus_kind_disc.rs index b2498463..2ef79795 100644 --- a/crates/sylpheed-formats/tests/ui_focus_kind_disc.rs +++ b/crates/sylpheed-formats/tests/ui_focus_kind_disc.rs @@ -17,25 +17,12 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; +mod common; +use common::disc_root; + const DECL_TABLE_AT: usize = 0x20; const DECL_ENTRY: usize = 60; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} - fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) .expect("dat/") diff --git a/crates/sylpheed-formats/tests/ui_header_time_disc.rs b/crates/sylpheed-formats/tests/ui_header_time_disc.rs index 34fe86b5..cd4aef58 100644 --- a/crates/sylpheed-formats/tests/ui_header_time_disc.rs +++ b/crates/sylpheed-formats/tests/ui_header_time_disc.rs @@ -16,21 +16,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) diff --git a/crates/sylpheed-formats/tests/ui_opt_link_disc.rs b/crates/sylpheed-formats/tests/ui_opt_link_disc.rs index d1c54833..2dcd0d1b 100644 --- a/crates/sylpheed-formats/tests/ui_opt_link_disc.rs +++ b/crates/sylpheed-formats/tests/ui_opt_link_disc.rs @@ -17,21 +17,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) diff --git a/crates/sylpheed-formats/tests/ui_paint_order_disc.rs b/crates/sylpheed-formats/tests/ui_paint_order_disc.rs index d997bf72..3ca70008 100644 --- a/crates/sylpheed-formats/tests/ui_paint_order_disc.rs +++ b/crates/sylpheed-formats/tests/ui_paint_order_disc.rs @@ -21,30 +21,8 @@ use sylpheed_formats::{ ui_layout::{self, ComposeOptions}, }; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; /// Every parseable screen build on the disc, as (pak name, bundle bytes). fn builds(root: &Path) -> Vec<(String, Vec)> { diff --git a/crates/sylpheed-formats/tests/ui_prm_primitives_disc.rs b/crates/sylpheed-formats/tests/ui_prm_primitives_disc.rs index f117cce5..fa55f0b2 100644 --- a/crates/sylpheed-formats/tests/ui_prm_primitives_disc.rs +++ b/crates/sylpheed-formats/tests/ui_prm_primitives_disc.rs @@ -9,21 +9,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) diff --git a/crates/sylpheed-formats/tests/ui_screen_vs_fragment_disc.rs b/crates/sylpheed-formats/tests/ui_screen_vs_fragment_disc.rs index 9e8bcec7..f2d63c37 100644 --- a/crates/sylpheed-formats/tests/ui_screen_vs_fragment_disc.rs +++ b/crates/sylpheed-formats/tests/ui_screen_vs_fragment_disc.rs @@ -21,21 +21,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) diff --git a/crates/sylpheed-formats/tests/ui_surfaces_disc.rs b/crates/sylpheed-formats/tests/ui_surfaces_disc.rs index 5acb1e5f..fdd1a024 100644 --- a/crates/sylpheed-formats/tests/ui_surfaces_disc.rs +++ b/crates/sylpheed-formats/tests/ui_surfaces_disc.rs @@ -10,30 +10,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{lsta, pak::PakArchive, ratc, t8ad}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; /// Every entry of every pak, plus every RATC child, as raw bytes. fn for_each_blob(root: &Path, mut f: impl FnMut(&str, &str, &[u8])) { diff --git a/crates/sylpheed-formats/tests/unit_layout_disc.rs b/crates/sylpheed-formats/tests/unit_layout_disc.rs index 40012e83..575c2a1e 100644 --- a/crates/sylpheed-formats/tests/unit_layout_disc.rs +++ b/crates/sylpheed-formats/tests/unit_layout_disc.rs @@ -11,6 +11,9 @@ use sylpheed_formats::idxd::IdxdObject; use sylpheed_formats::pak::PakArchive; use sylpheed_formats::unit_layout::{fields, Kind}; +mod common; +use common::disc_root; + /// Live objects identified in the dump, with the disc record each one is. /// `bf001` is here because full-record agreement is what identified it: the /// four-value signature also fitted `UN_be005_ADAN_SpaceFortress`, which @@ -31,19 +34,6 @@ const IDENTIFIED: &[(&str, &str)] = &[ const DUMP: &str = include_str!("../../../docs/re/captures/stage02-live-unit-definitions-deep.txt"); -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - if std::path::Path::new(&p).join("dat").is_dir() { - return Some(p); - } - } - let d = "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)"; - std::path::Path::new(d) - .join("dat") - .is_dir() - .then(|| d.to_string()) -} - /// `va -> offset -> value`, from the dump's `addr +off hex u32 f32` columns. fn live() -> BTreeMap> { let mut out: BTreeMap> = BTreeMap::new(); @@ -70,7 +60,8 @@ fn mapped_fields_match_the_disc_records() { eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); return; }; - let pak = PakArchive::open(format!("{disc}/dat/GP_MAIN_GAME_E.pak")).expect("main pak"); + let pak = + PakArchive::open(format!("{}/dat/GP_MAIN_GAME_E.pak", disc.display())).expect("main pak"); let live = live(); let floats: Vec<_> = fields() .into_iter() diff --git a/docker/decoder/bin/propose-work b/docker/decoder/bin/propose-work new file mode 100755 index 00000000..7ded18f2 --- /dev/null +++ b/docker/decoder/bin/propose-work @@ -0,0 +1,162 @@ +#!/usr/bin/env bash +# Push the branch, open the pull request, and move the issue to +# `state/needs-human` — the three steps PROTOCOL.md requires, as one command. +# +# Why this exists: +# +# `push-work` does the first third. GITEA-SETUP.md's own words are "the other +# two thirds being manual is how they get skipped", and both loop briefs had +# to carry a warning about it. A rule that depends on remembering three steps +# is a rule that decays; this makes the sequence structural instead. +# +# It does NOT reimplement push-work's refusals — it CALLS push-work, so `main`, +# shared branches and force-push stay refused in exactly one place. Duplicating +# them would let the two copies drift, and the copy that drifts is the one that +# matters. +# +# PROTOCOL.md §Pull requests: +# * branch `auto//-`, one item per branch +# * open the PR with `Closes #` in the body +# * label the issue `state/needs-human` and say, in one line, what to look at +# +# propose-work -m "what to look at" issue number from the branch +# propose-work -i 12 -m "..." -t "title" explicit +# propose-work -m "..." --dry-run print every call, make none +# +# The token is read from a file and passed to curl through a --config document +# on stdin. It is never an argument, never exported, never logged: arguments are +# world-readable in /proc, and this token can push. +set -euo pipefail + +DRY=0; ISSUE=""; TITLE=""; LOOK="" +while [ $# -gt 0 ]; do + case "$1" in + -i|--issue) ISSUE="${2:-}"; shift 2 ;; + -t|--title) TITLE="${2:-}"; shift 2 ;; + -m|--look) LOOK="${2:-}"; shift 2 ;; + --dry-run) DRY=1; shift ;; + -h|--help) sed -n '2,28p' "$0"; exit 0 ;; + *) echo "propose-work: unknown argument '$1'" >&2; exit 1 ;; + esac +done + +here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +repo_root=$(git rev-parse --show-toplevel) || exit 1 +cd "$repo_root" +branch=$(git rev-parse --abbrev-ref HEAD) + +# ── the issue number ──────────────────────────────────────────────────────── +# PROTOCOL names the branch `auto//-`, so the number is +# already there. Deriving it means the PR cannot cite a different issue than the +# branch was cut for -- a mismatch nobody would notice in review. +if [ -z "$ISSUE" ]; then + ISSUE=$(printf '%s\n' "$branch" | sed -n 's|^auto/[^/]*/\([0-9]\{1,\}\)-.*$|\1|p') +fi +if [ -z "$ISSUE" ]; then + echo "propose-work: no issue number." >&2 + echo " Either name the branch auto//-, or pass -i ." >&2 + exit 1 +fi + +# ── the "what to look at" line is NOT optional ────────────────────────────── +# PROTOCOL: an issue in `state/needs-human` "must say what to look at and what +# pass and fail look like, so a person can judge it in under a minute". An item +# that arrives without that sentence costs a human a round trip, so refuse here +# rather than let the label carry an empty promise. +if [ -z "$LOOK" ]; then + echo "propose-work: -m is required." >&2 + echo " state/needs-human means a person will look. Tell them what at, and" >&2 + echo " what pass and fail look like, in one line." >&2 + exit 1 +fi + +# The FIRST commit on the branch, not the last: PROTOCOL is one item per +# branch, so the opening commit names the unit while HEAD may well be "fix +# typo". Falls back to HEAD when the branch has no unique commits. +[ -n "$TITLE" ] || TITLE=$(git log --format=%s --reverse "origin/main..HEAD" 2>/dev/null | head -1) +[ -n "$TITLE" ] || TITLE=$(git log --format=%s -1) + +# ── credentials ───────────────────────────────────────────────────────────── +TOKFILE="${GITEA_TOKEN_FILE:-$HOME/.sylph-gitea-token}" +# Only when it will actually be used. `--dry-run` exists so an agent can check +# the command it is about to run; demanding a credential it never sends would +# make the check unavailable exactly where it is cheapest. +if [ "$DRY" = 0 ] && [ ! -s "$TOKFILE" ]; then + echo "propose-work: no Gitea token at $TOKFILE" >&2 + echo " The host must start the container with SYLPH_GITEA_TOKEN set." >&2 + exit 1 +fi + +remote=$(git remote get-url origin) +slug=$(printf '%s\n' "$remote" | sed -E 's|^.*://[^/]*/||; s|\.git$||') +API="https://$(printf '%s\n' "$remote" | sed -E 's|^.*://([^/@]*@)?([^/]*)/.*$|\2|')/api/v1" + +# curl with the credential supplied out-of-band. `--config -` reads a document +# from stdin; the token never reaches argv or the environment. +api() { # api [JSON] + local method="$1" path="$2" data="${3:-}" + { printf 'header = "Authorization: token %s"\n' "$(cat "$TOKFILE")" + printf 'header = "Content-Type: application/json"\n' + printf 'silent\nshow-error\nfail-with-body\nrequest = "%s"\n' "$method" + [ -n "$data" ] && printf 'data = %s\n' "$(printf '%s' "$data" | python3 -c 'import json,sys; print(json.dumps(sys.stdin.read()))')" + printf 'url = "%s%s"\n' "$API" "$path" + } | curl --config - +} + +echo "propose-work: branch=$branch issue=#$ISSUE repo=$slug" + +if [ "$DRY" = 1 ]; then + echo " would: push-work" + echo " would: POST /repos/$slug/pulls head=$branch base=main" + echo " title: $TITLE" + echo " body: Closes #$ISSUE + $LOOK" + echo " would: PATCH labels on #$ISSUE -> state/needs-human (dropping other state/*)" + echo " would: POST /repos/$slug/issues/$ISSUE/comments (the look-at line)" + echo "propose-work: --dry-run, nothing sent" + exit 0 +fi + +# ── 1. push (refusals live in push-work, not here) ────────────────────────── +"$here/push-work" + +# ── 2. the pull request ───────────────────────────────────────────────────── +body=$(printf '%s\n\nCloses #%s\n\n**What to look at:** %s\n' "$TITLE" "$ISSUE" "$LOOK") +payload=$(python3 - "$TITLE" "$branch" "$body" <<'PY' +import json,sys +print(json.dumps({"title":sys.argv[1],"head":sys.argv[2],"base":"main","body":sys.argv[3]})) +PY +) +if out=$(api POST "/repos/$slug/pulls" "$payload" 2>&1); then + num=$(printf '%s' "$out" | python3 -c 'import json,sys; print(json.load(sys.stdin)["number"])' 2>/dev/null || echo "?") + echo "propose-work: opened PR #$num" +else + # A second run after a fixup should not fail; the branch already has a PR. + case "$out" in + *"already exists"*) echo "propose-work: a pull request for $branch already exists — continuing" ;; + *) echo "propose-work: opening the PR failed:" >&2; echo "$out" >&2; exit 1 ;; + esac +fi + +# ── 3. move the issue to state/needs-human ────────────────────────────────── +# "Move", not "add": leaving state/in-progress on it makes the board lie about +# what is waiting on a person. +labels=$(api GET "/repos/$slug/labels?limit=100") +want=$(printf '%s' "$labels" | python3 -c 'import json,sys; print([l["id"] for l in json.load(sys.stdin) if l["name"]=="state/needs-human"][0])') +cur=$(api GET "/repos/$slug/issues/$ISSUE/labels") +drop=$(printf '%s' "$cur" | python3 -c ' +import json,sys +print(" ".join(str(l["id"]) for l in json.load(sys.stdin) + if l["name"].startswith("state/") and l["name"]!="state/needs-human"))') +for id in $drop; do api DELETE "/repos/$slug/issues/$ISSUE/labels/$id" >/dev/null; done +api POST "/repos/$slug/issues/$ISSUE/labels" "{\"labels\":[$want]}" >/dev/null +echo "propose-work: #$ISSUE -> state/needs-human" + +# ── 4. say what to look at, on the issue itself ───────────────────────────── +# The PR body has it too, but a person triaging the board reads issues. +cbody=$(python3 - "$LOOK" "$branch" <<'PY' +import json,sys +print(json.dumps({"body":"**Ready for a look.** %s\n\nBranch `%s`." % (sys.argv[1], sys.argv[2])})) +PY +) +api POST "/repos/$slug/issues/$ISSUE/comments" "$cbody" >/dev/null +echo "propose-work: done" diff --git a/docker/port/bin/propose-work b/docker/port/bin/propose-work new file mode 100755 index 00000000..7ded18f2 --- /dev/null +++ b/docker/port/bin/propose-work @@ -0,0 +1,162 @@ +#!/usr/bin/env bash +# Push the branch, open the pull request, and move the issue to +# `state/needs-human` — the three steps PROTOCOL.md requires, as one command. +# +# Why this exists: +# +# `push-work` does the first third. GITEA-SETUP.md's own words are "the other +# two thirds being manual is how they get skipped", and both loop briefs had +# to carry a warning about it. A rule that depends on remembering three steps +# is a rule that decays; this makes the sequence structural instead. +# +# It does NOT reimplement push-work's refusals — it CALLS push-work, so `main`, +# shared branches and force-push stay refused in exactly one place. Duplicating +# them would let the two copies drift, and the copy that drifts is the one that +# matters. +# +# PROTOCOL.md §Pull requests: +# * branch `auto//-`, one item per branch +# * open the PR with `Closes #` in the body +# * label the issue `state/needs-human` and say, in one line, what to look at +# +# propose-work -m "what to look at" issue number from the branch +# propose-work -i 12 -m "..." -t "title" explicit +# propose-work -m "..." --dry-run print every call, make none +# +# The token is read from a file and passed to curl through a --config document +# on stdin. It is never an argument, never exported, never logged: arguments are +# world-readable in /proc, and this token can push. +set -euo pipefail + +DRY=0; ISSUE=""; TITLE=""; LOOK="" +while [ $# -gt 0 ]; do + case "$1" in + -i|--issue) ISSUE="${2:-}"; shift 2 ;; + -t|--title) TITLE="${2:-}"; shift 2 ;; + -m|--look) LOOK="${2:-}"; shift 2 ;; + --dry-run) DRY=1; shift ;; + -h|--help) sed -n '2,28p' "$0"; exit 0 ;; + *) echo "propose-work: unknown argument '$1'" >&2; exit 1 ;; + esac +done + +here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +repo_root=$(git rev-parse --show-toplevel) || exit 1 +cd "$repo_root" +branch=$(git rev-parse --abbrev-ref HEAD) + +# ── the issue number ──────────────────────────────────────────────────────── +# PROTOCOL names the branch `auto//-`, so the number is +# already there. Deriving it means the PR cannot cite a different issue than the +# branch was cut for -- a mismatch nobody would notice in review. +if [ -z "$ISSUE" ]; then + ISSUE=$(printf '%s\n' "$branch" | sed -n 's|^auto/[^/]*/\([0-9]\{1,\}\)-.*$|\1|p') +fi +if [ -z "$ISSUE" ]; then + echo "propose-work: no issue number." >&2 + echo " Either name the branch auto//-, or pass -i ." >&2 + exit 1 +fi + +# ── the "what to look at" line is NOT optional ────────────────────────────── +# PROTOCOL: an issue in `state/needs-human` "must say what to look at and what +# pass and fail look like, so a person can judge it in under a minute". An item +# that arrives without that sentence costs a human a round trip, so refuse here +# rather than let the label carry an empty promise. +if [ -z "$LOOK" ]; then + echo "propose-work: -m is required." >&2 + echo " state/needs-human means a person will look. Tell them what at, and" >&2 + echo " what pass and fail look like, in one line." >&2 + exit 1 +fi + +# The FIRST commit on the branch, not the last: PROTOCOL is one item per +# branch, so the opening commit names the unit while HEAD may well be "fix +# typo". Falls back to HEAD when the branch has no unique commits. +[ -n "$TITLE" ] || TITLE=$(git log --format=%s --reverse "origin/main..HEAD" 2>/dev/null | head -1) +[ -n "$TITLE" ] || TITLE=$(git log --format=%s -1) + +# ── credentials ───────────────────────────────────────────────────────────── +TOKFILE="${GITEA_TOKEN_FILE:-$HOME/.sylph-gitea-token}" +# Only when it will actually be used. `--dry-run` exists so an agent can check +# the command it is about to run; demanding a credential it never sends would +# make the check unavailable exactly where it is cheapest. +if [ "$DRY" = 0 ] && [ ! -s "$TOKFILE" ]; then + echo "propose-work: no Gitea token at $TOKFILE" >&2 + echo " The host must start the container with SYLPH_GITEA_TOKEN set." >&2 + exit 1 +fi + +remote=$(git remote get-url origin) +slug=$(printf '%s\n' "$remote" | sed -E 's|^.*://[^/]*/||; s|\.git$||') +API="https://$(printf '%s\n' "$remote" | sed -E 's|^.*://([^/@]*@)?([^/]*)/.*$|\2|')/api/v1" + +# curl with the credential supplied out-of-band. `--config -` reads a document +# from stdin; the token never reaches argv or the environment. +api() { # api [JSON] + local method="$1" path="$2" data="${3:-}" + { printf 'header = "Authorization: token %s"\n' "$(cat "$TOKFILE")" + printf 'header = "Content-Type: application/json"\n' + printf 'silent\nshow-error\nfail-with-body\nrequest = "%s"\n' "$method" + [ -n "$data" ] && printf 'data = %s\n' "$(printf '%s' "$data" | python3 -c 'import json,sys; print(json.dumps(sys.stdin.read()))')" + printf 'url = "%s%s"\n' "$API" "$path" + } | curl --config - +} + +echo "propose-work: branch=$branch issue=#$ISSUE repo=$slug" + +if [ "$DRY" = 1 ]; then + echo " would: push-work" + echo " would: POST /repos/$slug/pulls head=$branch base=main" + echo " title: $TITLE" + echo " body: Closes #$ISSUE + $LOOK" + echo " would: PATCH labels on #$ISSUE -> state/needs-human (dropping other state/*)" + echo " would: POST /repos/$slug/issues/$ISSUE/comments (the look-at line)" + echo "propose-work: --dry-run, nothing sent" + exit 0 +fi + +# ── 1. push (refusals live in push-work, not here) ────────────────────────── +"$here/push-work" + +# ── 2. the pull request ───────────────────────────────────────────────────── +body=$(printf '%s\n\nCloses #%s\n\n**What to look at:** %s\n' "$TITLE" "$ISSUE" "$LOOK") +payload=$(python3 - "$TITLE" "$branch" "$body" <<'PY' +import json,sys +print(json.dumps({"title":sys.argv[1],"head":sys.argv[2],"base":"main","body":sys.argv[3]})) +PY +) +if out=$(api POST "/repos/$slug/pulls" "$payload" 2>&1); then + num=$(printf '%s' "$out" | python3 -c 'import json,sys; print(json.load(sys.stdin)["number"])' 2>/dev/null || echo "?") + echo "propose-work: opened PR #$num" +else + # A second run after a fixup should not fail; the branch already has a PR. + case "$out" in + *"already exists"*) echo "propose-work: a pull request for $branch already exists — continuing" ;; + *) echo "propose-work: opening the PR failed:" >&2; echo "$out" >&2; exit 1 ;; + esac +fi + +# ── 3. move the issue to state/needs-human ────────────────────────────────── +# "Move", not "add": leaving state/in-progress on it makes the board lie about +# what is waiting on a person. +labels=$(api GET "/repos/$slug/labels?limit=100") +want=$(printf '%s' "$labels" | python3 -c 'import json,sys; print([l["id"] for l in json.load(sys.stdin) if l["name"]=="state/needs-human"][0])') +cur=$(api GET "/repos/$slug/issues/$ISSUE/labels") +drop=$(printf '%s' "$cur" | python3 -c ' +import json,sys +print(" ".join(str(l["id"]) for l in json.load(sys.stdin) + if l["name"].startswith("state/") and l["name"]!="state/needs-human"))') +for id in $drop; do api DELETE "/repos/$slug/issues/$ISSUE/labels/$id" >/dev/null; done +api POST "/repos/$slug/issues/$ISSUE/labels" "{\"labels\":[$want]}" >/dev/null +echo "propose-work: #$ISSUE -> state/needs-human" + +# ── 4. say what to look at, on the issue itself ───────────────────────────── +# The PR body has it too, but a person triaging the board reads issues. +cbody=$(python3 - "$LOOK" "$branch" <<'PY' +import json,sys +print(json.dumps({"body":"**Ready for a look.** %s\n\nBranch `%s`." % (sys.argv[1], sys.argv[2])})) +PY +) +api POST "/repos/$slug/issues/$ISSUE/comments" "$cbody" >/dev/null +echo "propose-work: done" diff --git a/docs/agents/GITEA-SETUP.md b/docs/agents/GITEA-SETUP.md index 45608eda..2a51a33b 100644 --- a/docs/agents/GITEA-SETUP.md +++ b/docs/agents/GITEA-SETUP.md @@ -363,10 +363,12 @@ docker/port/sylph-port Not blockers for Phase 7, but the workflow is not finished without them: -* **`propose-work`**, superseding `push-work` — push the branch *and* open the PR - with `Closes #N` *and* set the label, in one step. Today `push-work` does the - first third; the other two thirds being manual is how they get skipped. Its - existing refusals stay: no `main`, no force-push. +* ~~**`propose-work`**~~ — done, as `docker/{decoder,port}/bin/propose-work`: + pushes through `push-work` (so its refusals stay in one place — no `main`, no + force-push), opens the PR with `Closes #N` taken from the branch name, and moves + the issue to `state/needs-human`. What is still missing is an *end-to-end* run: + everything before the network calls is exercised, the `POST`s are not, because + only the agent box holds a token that can make them. Its first real use is its test. * **an attachment uploader** — the MCP exposes `attachment_read` only, so putting a screenshot on an issue needs a direct `POST /repos/{owner}/{repo}/issues/{index}/assets`. * ~~**`gitea-verify`**~~ — done, as `tools/gitea-protect --verify`: asserts every diff --git a/docs/agents/decoder-loop.md b/docs/agents/decoder-loop.md index 4a0109df..04a6804a 100644 --- a/docs/agents/decoder-loop.md +++ b/docs/agents/decoder-loop.md @@ -95,6 +95,10 @@ Treat it as a fast index into 9.2 MB of machine code, not as a source of truth. ## Each iteration 1. **Read your notifications**, then `git fetch origin && git merge origin/main`. + Notifications are **polled — nothing pushes them.** + Then **look at your own open PRs.** A PR of yours with a red or still- + pending check is your next unit of work, *ahead of any new question*. You + opened it and stopped; nobody else is going to make it green. 2. **Pick one question** — the highest-priority `state/approved` item. Mid- question? Continue it. 3. **Do the smallest experiment that could settle it**, and try to *refute* your @@ -113,6 +117,20 @@ Treat it as a fast index into 9.2 MB of machine code, not as a source of truth. 7. **Commit, `push-work`, open the PR**, label the issue `state/needs-human`, and **stop.** One unit per iteration; do not stack a second on an unverified first. +## Before you call a merge resolved + +**Check the workspace, not the crate you touched** — `cargo check --workspace +--all-targets`. + +🔴 A merge can break a crate that neither side edited. PR #23 resolved a +conflict in `sylpheed-cli`, confirmed `cargo build -p sylpheed-cli`, and went +red on `sylpheed-viewer` — where one branch had added a struct field and the +other had a literal construction of that struct. Neither file was in the +conflict, so nothing in the merge pointed at it. + +The crate you fixed is the one place the break *cannot* be, because you just +looked at it. + ## Hard rules * **Do not build the port.** No Godot, no exporter, no transcoding. diff --git a/docs/agents/port-loop.md b/docs/agents/port-loop.md index 3cd25bdb..977d0dec 100644 --- a/docs/agents/port-loop.md +++ b/docs/agents/port-loop.md @@ -98,6 +98,9 @@ written down. Steps 1 and 8 are the ones that were missing entirely. 1. **Read your notifications**, then `git fetch origin && git merge origin/main`. Notifications are **polled — nothing pushes them.** If you skip this, nothing addressed to you ever arrives, and it looks like silence rather than an error. + Then **look at your own open PRs.** A PR of yours with a red or still- + pending check is your next unit of work, *ahead of any new item*. You opened + it and stopped; nobody else is going to make it green. 2. **Pick one item** — the highest-priority `state/approved` issue that is yours. Mid-item? Continue it. 3. **Build the smallest thing that reaches a gate.** A gate is an **artifact**, @@ -122,6 +125,20 @@ written down. Steps 1 and 8 are the ones that were missing entirely. set the label. Those are two separate MCP calls (`pull_request_write`, `issue_write`), and they are the two most commonly skipped steps in this list. +## Before you call a merge resolved + +**Check the workspace, not the crate you touched** — `cargo check --workspace +--all-targets`. + +🔴 A merge can break a crate that neither side edited. PR #23 resolved a +conflict in `sylpheed-cli`, confirmed `cargo build -p sylpheed-cli`, and went +red on `sylpheed-viewer` — where one branch had added a struct field and the +other had a literal construction of that struct. Neither file was in the +conflict, so nothing in the merge pointed at it. + +The crate you fixed is the one place the break *cannot* be, because you just +looked at it. + ## The wall The Godot project **never reads a disc format**. No IPFB, RATC, T8aD, XMA or WMV. diff --git a/justfile b/justfile index c79d5829..4034b45f 100644 --- a/justfile +++ b/justfile @@ -71,6 +71,28 @@ sniff-unknown: test: cargo test --workspace +# Run the disc-backed suites. They are gated on the environment ALONE now (#16): +# no source file hardcodes a path any more, so nothing runs by accident because +# a machine happens to have a directory. Put your paths in `.env` (already +# gitignored as a local dev override): +# +# SYLPHEED_DISC="/path/to/extracted/disc" # the dir containing dat/ +# +# QUOTE the values. These paths contain spaces, and an unquoted `VAR=a b c` +# is parsed as "run command b with VAR=a" -- it fails silently into ABSENT. +# SYLPHEED_RES3D=/path/to/hidden/resource3d +# SYLPHEED_ISO=/path/to/game.iso +# +# Read the corpus block the run prints -- the pass/fail tally is identical +# whether or not the corpus was exercised, which is the whole of #16. +test-disc: + #!/usr/bin/env bash + set -euo pipefail + [ -f .env ] && set -a && . ./.env && set +a + cargo test --workspace + echo "--- corpus report ---" + cat target/sylpheed-corpus-report.txt + # Run tests including ISO integration tests (requires SYLPHEED_ISO env var) test-integration: SYLPHEED_ISO=./game.iso cargo test --workspace -- --include-ignored