From b924d1e9cba529432159eee8bd31251caa131308 Mon Sep 17 00:00:00 2001 From: Fabian Hamm Date: Thu, 10 Sep 2026 19:17:41 +0200 Subject: [PATCH 1/4] =?UTF-8?q?feat:=20propose-work=20=E2=80=94=20push,=20?= =?UTF-8?q?open=20the=20PR,=20and=20move=20the=20issue,=20in=20one=20comma?= =?UTF-8?q?nd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GITEA-SETUP.md has listed this under "Still to build" since 2026-09-04: `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. PROTOCOL.md 145-149 requires all three: * 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 Until now those were three things to remember, and PR #20 had to add a warning to `port-loop.md` about the two that get forgotten. A rule enforced by memory decays; this makes the sequence structural. WHAT IT DOES NOT DO is 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 copies drift, and the copy that drifts is the one that matters. Three design choices worth stating: * THE ISSUE NUMBER IS DERIVED FROM THE BRANCH NAME, which PROTOCOL already specifies as `auto//-`. So a PR cannot cite a different issue than the branch was cut for -- a mismatch no reviewer would catch. `-i` overrides. * `-m` IS MANDATORY. PROTOCOL says 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". Refusing without that line is cheaper than letting the label carry an empty promise and costing a human a round trip. * IT MOVES THE LABEL RATHER THAN ADDING IT -- other `state/*` labels are removed. Leaving `state/in-progress` attached makes the board lie about what is waiting on a person. The token is read from a file and handed to curl through a `--config` document on stdin: never an argument, never exported. Arguments are world-readable in /proc and this token can push. Verified that the mechanism actually delivers the header rather than silently dropping it -- with a bogus token the API answers "invalid username, password or token", while the same URL with no header returns the list anonymously, so the header is demonstrably being read. Also verified: issue derived from the branch (#42 from `auto/decoder/42-widget-census`), refusal without `-m`, refusal when the branch carries no number, `--dry-run` sends nothing, and repo/API derivation from the remote. `--dry-run` deliberately does NOT require a credential -- it exists so an agent can check the command it is about to run, and demanding a token it never sends would make the check unavailable exactly where it is cheapest. Identical in both agents' bin/ on purpose: the agent name comes from the branch, so there is nothing per-agent to diverge. UNVERIFIED, and stated as such: no end-to-end run. That needs a real token and a real issue, which this desktop does not have -- it is the second machine, and the agent credentials live on the agent box. Everything above the network call is exercised; the POSTs are not. Refs #11 Co-Authored-By: Claude Opus 5 --- docker/decoder/bin/propose-work | 162 ++++++++++++++++++++++++++++++++ docker/port/bin/propose-work | 162 ++++++++++++++++++++++++++++++++ 2 files changed, 324 insertions(+) create mode 100755 docker/decoder/bin/propose-work create mode 100755 docker/port/bin/propose-work 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" From 15d51b30aca6cdb7625c4fc16b1c9d5f4cc92a5c Mon Sep 17 00:00:00 2001 From: Fabian Hamm Date: Thu, 10 Sep 2026 20:42:44 +0200 Subject: [PATCH 2/4] test(formats): make $SYLPHEED_DISC an actual control (#16 remedy 3) Before this commit, `unset SYLPHEED_DISC` did not disable the disc-backed suites on the machine that has the disc: every `disc_root()` fell back to a hardcoded absolute path that exists on this box. The env var looked like a control and was not one. Same for $SYLPHEED_RES3D and $SYLPHEED_ISO. Replace the duplicated resolvers with one `tests/common/mod.rs`: - 17 local `disc_root()` definitions -> 1 - 7 copies of the skip macro -> 1 (`skip_without_disc!` and siblings) - 16 hardcoded absolute paths -> 0 executable ones (3 of those were inline in `mesh_disc.rs`, in no resolver at all, and 2 were in `examples/`) - `corpus_report.rs` now reports on the SAME resolver the suites use, instead of a second copy of the logic its own comments flagged as a drift risk. The 17 copies had already drifted into FIVE variants, and they were not all the same function. `movie_manifest_disc`, `movie_subtitle_disc` and `slb_disc` honoured $SYLPHEED_DISC and nothing else, while the other 14 fell back. So one name already meant two things -- a third instance of the shape #16 is about. The shared helper adopts the env-only behaviour those three already had, rather than inventing a sixth variant. Two module docs still described the fallback after it was deleted, which is the same defect in prose: `texture_disc` claimed "or the default dev path exists" and `pak_idxd_disc` said "or drop it at the default dev path below". Both now say what the code does. Verified both ways on the machine that HAS the corpus, which is the only place this refactor can be falsified: A env unset -> "ABSENT -- $SYLPHEED_DISC unset; its suites self-skip" suites=31 passed=209 failed=0 ignored=14, slowest 0.12s B env set -> "PRESENT via $SYLPHEED_DISC" (all three corpora) suites=31 passed=209 failed=0 ignored=14, slowest 1235.53s (mesh_consistency_disc) Identical tallies, opposite corpus states, ~10000x apart in wall clock. (A) is new behaviour -- it was previously unreachable here. (B) proves nothing broke. `just test-disc` sources `.env` (already gitignored) for the set case. Note the quoting trap documented there: the corpus paths contain spaces, and an unquoted `VAR=a b c` parses as "run command `b`", failing silently into ABSENT -- which looks exactly like a working skip. Remedy (1) (`#[ignore]` + `--ignored`) is deliberately NOT done here: (3) already moves the mode from the filesystem into the environment, and `#[ignore]` already carries three meanings in this directory (corpus-absent, known-failing, bare). Overloading it a fourth time would re-create the defect. `cargo fmt --all --check` clean; no new compiler warnings. Refs #16 Co-Authored-By: Claude Opus 5 --- .../examples/default_owners.rs | 8 +- .../examples/defaulted_fields.rs | 7 +- .../tests/caption_families_disc.rs | 24 +-- crates/sylpheed-formats/tests/common/mod.rs | 79 +++++++++ .../sylpheed-formats/tests/corpus_report.rs | 160 +++++++----------- .../tests/idxd_records_disc.rs | 23 +-- .../tests/ixud_records_disc.rs | 23 +-- .../tests/mesh_consistency_disc.rs | 19 +-- crates/sylpheed-formats/tests/mesh_disc.rs | 39 ++--- .../tests/movie_manifest_disc.rs | 6 +- .../tests/movie_subtitle_disc.rs | 8 +- .../sylpheed-formats/tests/pak_idxd_disc.rs | 34 +--- crates/sylpheed-formats/tests/slb_disc.rs | 6 +- .../tests/slb_leading_segment_disc.rs | 25 +-- crates/sylpheed-formats/tests/texture_disc.rs | 19 +-- .../tests/ui_focus_kind_disc.rs | 19 +-- .../tests/ui_header_time_disc.rs | 17 +- .../tests/ui_opt_link_disc.rs | 17 +- .../tests/ui_paint_order_disc.rs | 26 +-- .../tests/ui_prm_primitives_disc.rs | 17 +- .../tests/ui_screen_vs_fragment_disc.rs | 17 +- .../tests/ui_surfaces_disc.rs | 26 +-- .../tests/unit_layout_disc.rs | 19 +-- justfile | 22 +++ 24 files changed, 239 insertions(+), 421 deletions(-) create mode 100644 crates/sylpheed-formats/tests/common/mod.rs 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 3a554eea..379bf08d 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 cc0b73a4..0e6a4ea0 100644 --- a/crates/sylpheed-formats/tests/mesh_consistency_disc.rs +++ b/crates/sylpheed-formats/tests/mesh_consistency_disc.rs @@ -11,25 +11,12 @@ //! 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; -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 ea4ec8df..19d1aa72 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 bdea2406..36d03a68 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: &PathBuf) -> (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 e17b4e56..c584335b 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: &PathBuf, 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 c5e69f6b..a9204206 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 6e552ae9..0c8525ac 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 d9eb8e0b..3d7e7c65 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 7f280a32..2804792b 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 9b8e4357..6e3e0198 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/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 From 7f38c9d12e138e8eca9a423c03babd8340136e41 Mon Sep 17 00:00:00 2001 From: Fabian Hamm Date: Fri, 11 Sep 2026 06:14:02 +0200 Subject: [PATCH 3/4] =?UTF-8?q?docs:=20strike=20propose-work=20from=20"Sti?= =?UTF-8?q?ll=20to=20build"=20=E2=80=94=20it=20is=20built?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GITEA-SETUP.md listed `propose-work` as unbuilt, and this PR builds it without touching that list, so merging would leave the doc describing a tool as missing that exists. Strike it the way `gitea-verify` already is: say what was built, then say what is still missing — here, an end-to-end run, which only the agent box can do because only it holds a token that can make the POSTs. Co-Authored-By: Claude Opus 5 --- docs/agents/GITEA-SETUP.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 From b21e9cba8df294651463cd7b1cc75b87a74ce7c8 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Fri, 11 Sep 2026 22:04:57 +0200 Subject: [PATCH 4/4] docs(agents): a red PR is the next unit of work, and check the workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps the first post-migration decoder iteration exposed, both in the same place — the loop ends before it can observe its own result. 1. Step 1 read notifications but never looked at the agent's own open PRs. It opened #23, labelled the issue `state/needs-human`, and stopped; nothing in the brief would ever bring it back to a red check. Next iteration it planned to start a new question on top. 2. It called the merge resolved on `cargo build -p sylpheed-cli` — the crate the conflict was in. The break was in `sylpheed-viewer`, which neither side of the merge edited: one branch added a `ComposeOptions` field, the other had a literal construction of it. The crate you just fixed is the one place the breakage cannot be. Co-Authored-By: Claude Opus 5 --- docs/agents/decoder-loop.md | 18 ++++++++++++++++++ docs/agents/port-loop.md | 17 +++++++++++++++++ 2 files changed, 35 insertions(+) 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.