F1 starter sets 1/2: presets, resumable install tasks, preset CI check #26

Merged
claude-bot merged 19 commits from phase1/starter-sets-core into main 2026-10-22 08:17:39 +00:00
Collaborator

What

The core half of F1 starter sets, with no UI yet. It adds five starter sets (presets/*.toml: English, Deutsch, Français, original languages, study) that state each module's licence and sizes. It adds a SWORD installer in sword/ (CrossWire index, resumable rawzip downloads, install and remove), and a resumable task queue in core/: migration 002 and a runner thread that survives kill -9 mid-download. A weekly CI job checks the sets against CrossWire's live index. Part of #10 (part 1/2; part 2 is the module manager and first-run picker UI).

Blast radius

Tier: trunk
Trunk paths touched: sword/src/install*, core/src/db/ (002_tasks.sql, tasks.rs, db.rs), core/src/tasks/, presets/, Cargo.toml, deny.toml, Makefile.toml, .forgejo/workflows/presets.yml, tools/ci/preset-check/, qt/Cargo.lock
Testing tier: maximum (new dependencies: ureq/rustls, zip 9, toml, serde; a new CI workflow). Also elevated: downloads from the network, migration 002.
Gate / rollback: nothing is wired into the app yet, because the Qt glue does not start the runner or installer. Rollback is a revert of this merge. Migration 002 only adds the tasks table, and app.sqlite3 is backed up (VACUUM INTO) before it runs. The previous build would see schema version 2, so rolling back the app needs that .bak-v1.

Proof

  • Testing-tier gates. None are unrun.
    • Maximum (new dependencies, a new workflow):
      • Standard E2E: cargo make app-docker mem-check in CI's clickable image, run locally. This is the preview.
      • Targeted check: cargo-deny and cargo-audit pass on the new dependencies, which include zip 9 rather than 2.x because of RUSTSEC advisories.
      • The new preset-check job: a green CI run on c18f37e, and exit 1 against a deliberately broken copy of the presets.
    • Elevated targets, written down:
      • Network: CrossWire's public mirror, https://crosswire.org/ftpmirror/pub/sword. Only GET and HEAD reads, no writes. Installs run only against the local test server.
      • Migration 002: throwaway databases in temp dirs (concurrent_opens_of_an_old_file_all_migrate), and mem-check's fresh app.sqlite3. The app migrated, started and restored state on 3b616f1. No user database was touched.
      • Issue step: a local mock API. No real issues were touched.
  • Rust gates: cargo make exit 0 on c18f37e: fmt, clippy, tests with coverage (core lib 98 passed), release check, cargo-deny advisories ok, bans ok, licenses ok, sources ok, cargo-audit clean (root and qt).
  • App gates: cargo make app-docker exit 0 on the merged tree 3b616f1: start-up 134 ms, peak PSS 147478 kB (budget 165000), kill -9 restore ok, picker checks ok.
  • All installer and task tests use a local HTTP test server (sword/src/install/testserver.rs) that cuts connections, fails with statuses, changes files mid-download and ignores ranges. None of them touches the network.
  • Fails without the change:
    • sword mutants: Range offset 0, no If-Range, no If-None-Match, tar checksum skipped, conf before data, DataPath checks off, conf removal skipped.
    • core mutants: requeue no-op, no pending check, ORDER BY reversed, cancel deleting all states, Stopped→failed, a weakened name check, no clean on failure, a plain (non-unique) index, remove without cancel.
    • preset-check mutants: licence, size, lock and package checks removed.
    • Every review fix also has a test that fails on the old code: concurrent migration, the start_task hot loop, cancel on the last progress, finish before Done, the RawGenBook prefix, the cumulative unpack budget, and three resume paths.
  • Concurrent first open of a v1 database migrates once, from both connections: concurrent_opens_of_an_old_file_all_migrate.
  • Live check: cargo run --release -p utsb-preset-check gives ok: 5 sets, 23 modules match https://crosswire.org/ftpmirror/pub/sword. A deliberately broken copy exits 1 with 3 problems.
  • Network drops in the preset check (CI on 3b616f1 reported StrongsGreek: no rawzip ... Peer disconnected): a HEAD that gets no answer is retried 4 times. If it never answers, the job exits 2 ("could not check") and the remaining modules are not asked. Tests count the HEAD calls (2/4/1), and each fails without its change.
  • presets.yml failure step, local mock (no real issues touched): no open issue → creates one; a matching issue next to another → comments on the match; API 403 → non-fatal.
  • CI: rust, app, click, audit and preset-check (live CrossWire) all green on c18f37e, the last code commit.
  • Budgets (docs/07): no start-up work added, because nothing in the app uses the runner yet. Peak PSS is 147.5–154.0 MB against the 165 MB budget, within the noise of main. The new dependencies grow the linked size. Click and binary size were not measured; part 2 measures them once the glue links the installer. The preset TOMLs are embedded with include_str! (about 9 kB).
  • Not covered:
    • No 416 path (#28): a validator-less file that shrinks below the downloaded part repeats 416 until the retries end.
    • Two core cancel mutants are caught only by a test timeout, not an assertion.
    • The preset check's exit 2 for a package-host outage (#29) is unit-tested (check_live) but not at CLI level, because the test server cannot fail a HEAD at the network level.
    • When the check exits 2, problems already found for other modules are not printed (#29).
    • No arm64 phone run. Nothing user-visible yet; the phone check comes with part 2.

Review

Reviewer: fresh-context review at high (7950ada): 8 findings (2 medium, 6 low), all 8 fixed in 8bd9a64. Re-review at medium: 1 low (issue dedupe), fixed in 9aa196a. Coverage review at medium (209): 9 lows, 5 fixed in ce21b29, 1 deferred to #28 (no 416 path), 3 declined as by design: the test server has no Drop (harmless), presets.yml runs on PRs against live CrossWire (intended), and the winnow duplicate is a deny warning only. Delta review 213: 1 low (exact RawGenBook match), declined, because ModDrv values are fixed tokens. Delta review 214: 4 lows, 2 fixed in c18f37e, 2 deferred to #29. Delta review 215: no findings.
Review-Confidence: 55/100. A trunk-tier high review plus four delta reviews cover every code change, CI is green, and the auditor's own read of http.rs found no missed defect. Points off for the 416 resume gap and the preset-check error-handling notes (now #28, #29), and because four of the five presets/*.toml files were never listed as reviewed (the live check backs their contents).

Confidence by part

  • sword/src/install/http.rs (resumable download): high. Every resume path has a mutant-killing test against the cutting test server. Read it for the 416 gap.
  • sword/src/install/package.rs (rawzip install/remove, ustar reader): high. DataPath confinement, the byte budget and the conf-last order are each tested.
  • core/src/db.rs + 002 (migration lock): medium-high. The concurrent-open test covers the race, but the process-wide lock is new.
  • core/src/tasks/ (runner): medium. Two cancel paths are proven only by a timeout, and the runner is not wired into the app yet.
  • presets/*.toml sizes and licences: medium. They match CrossWire today, and the weekly job watches them.
  • .forgejo/workflows/presets.yml issue step: low. It was proven against a mock API only. Read it.

Follow-ups for part 2: wire the runner into qt/, add an index-fetch command, decide on an auto-retry policy, measure click size and PSS, and add assertion tests for the two hang-only mutants.

🤖 Generated with Claude Code

## What The core half of F1 starter sets, with no UI yet. It adds five starter sets (`presets/*.toml`: English, Deutsch, Français, original languages, study) that state each module's licence and sizes. It adds a SWORD installer in `sword/` (CrossWire index, resumable rawzip downloads, install and remove), and a resumable task queue in `core/`: migration 002 and a runner thread that survives kill -9 mid-download. A weekly CI job checks the sets against CrossWire's live index. Part of #10 (part 1/2; part 2 is the module manager and first-run picker UI). ## Blast radius **Tier:** trunk **Trunk paths touched:** `sword/src/install*`, `core/src/db/` (002_tasks.sql, tasks.rs, db.rs), `core/src/tasks/`, `presets/`, `Cargo.toml`, `deny.toml`, `Makefile.toml`, `.forgejo/workflows/presets.yml`, `tools/ci/preset-check/`, `qt/Cargo.lock` **Testing tier:** maximum (new dependencies: ureq/rustls, zip 9, toml, serde; a new CI workflow). Also elevated: downloads from the network, migration 002. **Gate / rollback:** nothing is wired into the app yet, because the Qt glue does not start the runner or installer. Rollback is a revert of this merge. Migration 002 only adds the `tasks` table, and `app.sqlite3` is backed up (`VACUUM INTO`) before it runs. The previous build would see schema version 2, so rolling back the app needs that `.bak-v1`. ## Proof - **Testing-tier gates.** None are unrun. - Maximum (new dependencies, a new workflow): - Standard E2E: `cargo make app-docker` mem-check in CI's clickable image, run locally. This is the preview. - Targeted check: `cargo-deny` and `cargo-audit` pass on the new dependencies, which include zip 9 rather than 2.x because of RUSTSEC advisories. - The new preset-check job: a green CI run on c18f37e, and exit 1 against a deliberately broken copy of the presets. - Elevated targets, written down: - Network: CrossWire's public mirror, `https://crosswire.org/ftpmirror/pub/sword`. Only GET and HEAD reads, no writes. Installs run only against the local test server. - Migration 002: throwaway databases in temp dirs (`concurrent_opens_of_an_old_file_all_migrate`), and mem-check's fresh `app.sqlite3`. The app migrated, started and restored state on 3b616f1. No user database was touched. - Issue step: a local mock API. No real issues were touched. - Rust gates: `cargo make` exit 0 on c18f37e: fmt, clippy, tests with coverage (core lib 98 passed), release check, cargo-deny `advisories ok, bans ok, licenses ok, sources ok`, cargo-audit clean (root and qt). - App gates: `cargo make app-docker` exit 0 on the merged tree 3b616f1: start-up 134 ms, peak PSS 147478 kB (budget 165000), kill -9 restore ok, picker checks ok. - All installer and task tests use a local HTTP test server (`sword/src/install/testserver.rs`) that cuts connections, fails with statuses, changes files mid-download and ignores ranges. None of them touches the network. - Fails without the change: - sword mutants: Range offset 0, no If-Range, no If-None-Match, tar checksum skipped, conf before data, DataPath checks off, conf removal skipped. - core mutants: requeue no-op, no pending check, ORDER BY reversed, cancel deleting all states, Stopped→failed, a weakened name check, no clean on failure, a plain (non-unique) index, remove without cancel. - preset-check mutants: licence, size, lock and package checks removed. - Every review fix also has a test that fails on the old code: concurrent migration, the start_task hot loop, cancel on the last progress, finish before Done, the RawGenBook prefix, the cumulative unpack budget, and three resume paths. - Concurrent first open of a v1 database migrates once, from both connections: `concurrent_opens_of_an_old_file_all_migrate`. - Live check: `cargo run --release -p utsb-preset-check` gives `ok: 5 sets, 23 modules match https://crosswire.org/ftpmirror/pub/sword`. A deliberately broken copy exits 1 with 3 problems. - Network drops in the preset check (CI on 3b616f1 reported `StrongsGreek: no rawzip ... Peer disconnected`): a HEAD that gets no answer is retried 4 times. If it never answers, the job exits 2 ("could not check") and the remaining modules are not asked. Tests count the HEAD calls (2/4/1), and each fails without its change. - presets.yml failure step, local mock (no real issues touched): no open issue → creates one; a matching issue next to another → comments on the match; API 403 → non-fatal. - CI: rust, app, click, audit and preset-check (live CrossWire) all green on c18f37e, the last code commit. - **Budgets (docs/07):** no start-up work added, because nothing in the app uses the runner yet. Peak PSS is 147.5–154.0 MB against the 165 MB budget, within the noise of main. The new dependencies grow the linked size. Click and binary size were not measured; part 2 measures them once the glue links the installer. The preset TOMLs are embedded with `include_str!` (about 9 kB). - **Not covered:** - No 416 path (#28): a validator-less file that shrinks below the downloaded part repeats 416 until the retries end. - Two core cancel mutants are caught only by a test timeout, not an assertion. - The preset check's exit 2 for a package-host outage (#29) is unit-tested (`check_live`) but not at CLI level, because the test server cannot fail a HEAD at the network level. - When the check exits 2, problems already found for other modules are not printed (#29). - No arm64 phone run. Nothing user-visible yet; the phone check comes with part 2. ## Review **Reviewer:** fresh-context review at high (7950ada): 8 findings (2 medium, 6 low), all 8 fixed in 8bd9a64. Re-review at medium: 1 low (issue dedupe), fixed in 9aa196a. Coverage review at medium (209): 9 lows, 5 fixed in ce21b29, 1 deferred to #28 (no 416 path), 3 declined as by design: the test server has no Drop (harmless), presets.yml runs on PRs against live CrossWire (intended), and the winnow duplicate is a deny warning only. Delta review 213: 1 low (exact `RawGenBook` match), declined, because ModDrv values are fixed tokens. Delta review 214: 4 lows, 2 fixed in c18f37e, 2 deferred to #29. Delta review 215: no findings. **Review-Confidence:** 55/100. A trunk-tier high review plus four delta reviews cover every code change, CI is green, and the auditor's own read of `http.rs` found no missed defect. Points off for the 416 resume gap and the preset-check error-handling notes (now #28, #29), and because four of the five `presets/*.toml` files were never listed as reviewed (the live check backs their contents). ## Confidence by part - `sword/src/install/http.rs` (resumable download): high. Every resume path has a mutant-killing test against the cutting test server. Read it for the 416 gap. - `sword/src/install/package.rs` (rawzip install/remove, ustar reader): high. DataPath confinement, the byte budget and the conf-last order are each tested. - `core/src/db.rs` + 002 (migration lock): medium-high. The concurrent-open test covers the race, but the process-wide lock is new. - `core/src/tasks/` (runner): medium. Two cancel paths are proven only by a timeout, and the runner is not wired into the app yet. - `presets/*.toml` sizes and licences: medium. They match CrossWire today, and the weekly job watches them. - `.forgejo/workflows/presets.yml` issue step: low. It was proven against a mock API only. Read it. Follow-ups for part 2: wire the runner into qt/, add an index-fetch command, decide on an auto-retry policy, measure click size and PSS, and add assertion tests for the two hang-only mutants. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Built on spike S4. Adds ureq (rustls) and zip 9 (zip 2 has open advisories),
and a deny.toml exception for webpki-roots' CDLA-Permissive-2.0 licence.
Plan in docs/superpowers/plans/2026-10-21-starter-sets-core.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
qt: lock the new core dependencies
Some checks failed
CI / rust (pull_request) Successful in 3m58s
Presets / preset-check (pull_request) Successful in 1m43s
CI / audit (pull_request) Successful in 9s
CI / app (pull_request) Successful in 2m2s
CI / click (pull_request) Has been cancelled
7950adaf15
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
claude-bot left a comment

Fresh-context review (posted by the author account)

Reviewed 7950adaf. Two medium and six low findings, inline below. Each gets an answer in its thread after the fix.

Fresh-context review (posted by the author account) Reviewed 7950adaf. Two medium and six low findings, inline below. Each gets an answer in its thread after the fix.
@ -0,0 +40,4 @@
/opt/rust/cargo/registry/cache
key: presets-deps-${{ hashFiles('Cargo.lock') }}
restore-keys: |
presets-deps-
Author
Collaborator

[low] The weekly schedule job fails silently when CrossWire changes a module. Nobody is notified unless the Actions page is opened, so a stale size or licence shown in the picker can persist for months. The docs describe the schedule as the safeguard: add a notification or open an issue on failure, or document where the result is read.

[low] The weekly schedule job fails silently when CrossWire changes a module. Nobody is notified unless the Actions page is opened, so a stale size or licence shown in the picker can persist for months. The docs describe the schedule as the safeguard: add a notification or open an issue on failure, or document where the result is read.
@ -113,6 +116,9 @@ impl Db {
fn try_open(path: &Path, steps: &[&str]) -> Result<Db> {
let mut conn = Connection::open(path)?;
// The task runner has its own connection: wait for a write lock
Author
Collaborator

[medium] The task runner and the reading worker open the same app.sqlite3. On the first launch after upgrading from schema 1, both read user_version=1 before either takes a write lock, both run the backup (VACUUM INTO deletes and recreates the same .bak-v1), and both run 002_tasks.sql. The loser fails with 'table tasks already exists' (or SQLITE_BUSY_SNAPSHOT), which is not is_corrupt, so Db::open returns Err. If the runner loses, it reports Event::Error and the thread ends: later sends are silently dropped and no install runs. If the reader loses, the app falls back to the in-memory DB and loses saved state. busy_timeout does not help because the version check is outside the transaction. Fix: BEGIN IMMEDIATE and re-read user_version inside it (or serialise the open); add a test that opens twice concurrently on a v1 file.

[medium] The task runner and the reading worker open the same app.sqlite3. On the first launch after upgrading from schema 1, both read user_version=1 before either takes a write lock, both run the backup (VACUUM INTO deletes and recreates the same .bak-v1), and both run 002_tasks.sql. The loser fails with 'table tasks already exists' (or SQLITE_BUSY_SNAPSHOT), which is not is_corrupt, so Db::open returns Err. If the runner loses, it reports Event::Error and the thread ends: later sends are silently dropped and no install runs. If the reader loses, the app falls back to the in-memory DB and loses saved state. busy_timeout does not help because the version check is outside the transaction. Fix: BEGIN IMMEDIATE and re-read user_version inside it (or serialise the open); add a test that opens twice concurrently on a v1 file.
@ -0,0 +173,4 @@
let _ = t.join();
}
}
}
Author
Collaborator

[low] If Db::open fails, run() emits Event::Error(0) and returns. The Runner stays alive with its receiver dropped, so send() silently discards every command and pause/resume do nothing. The picker would show a queued install that never starts, with no further error. Keep the thread alive and answer each command with an error, or expose a 'dead' state.

[low] If Db::open fails, run() emits Event::Error(0) and returns. The Runner stays alive with its receiver dropped, so send() silently discards every command and pause/resume do nothing. The picker would show a queued install that never starts, with no further error. Keep the thread alive and answer each command with an error, or expose a 'dead' state.
@ -0,0 +125,4 @@
if let Err(e) = self.db.start_task(task.id) {
return self.emit(Event::Error(format!("tasks: {e}")));
}
self.current = Some((module.clone(), false));
Author
Collaborator

[medium] If db.start_task fails (locked past the 5 s busy timeout, read-only, full disk), execute emits Event::Error and returns with the row still 'queued'. The run loop calls next_task, which returns the same row, and execute runs again: a hot loop emitting an Error per iteration, burning battery. Unlike the next_task error path it does not set paused. The same applies when a later fail_task/finish_task keeps failing. Pause the runner on this error, as next_task does.

[medium] If db.start_task fails (locked past the 5 s busy timeout, read-only, full disk), execute emits Event::Error and returns with the row still 'queued'. The run loop calls next_task, which returns the same row, and execute runs again: a hot loop emitting an Error per iteration, burning battery. Unlike the next_task error path it does not set paused. The same applies when a later fail_task/finish_task keeps failing. Pause the runner on this error, as next_task does.
@ -0,0 +205,4 @@
/// A download event: progress goes to the table and the listener, and
/// commands that arrived meanwhile are handled (a cancel stops it).
fn on_download(&mut self, rx: &Receiver<(u64, Command)>, task: &Task, ev: &install::Event) {
let progress = match *ev {
Author
Collaborator

[low] A Cancel drained in on_download during the final Event::Downloaded progress does not stop the download, which is already complete. install() goes on to package::install and installs the module. execute then overrides the outcome with Outcome::Cancelled, so the row is gone and no Done or Failed event is emitted, but the module is installed on disk and shows in the Bible list. Check the flag before package::install, or treat Done as authoritative.

[low] A Cancel drained in on_download during the final Event::Downloaded progress does not stop the download, which is already complete. install() goes on to package::install and installs the module. execute then overrides the outcome with Outcome::Cancelled, so the row is gone and no Done or Failed event is emitted, but the module is installed on disk and shows in the Bible list. Check the flag before package::install, or treat Done as authoritative.
@ -0,0 +45,4 @@
let agent = Agent::config_builder()
.timeout_connect(Some(Duration::from_secs(15)))
.timeout_recv_response(Some(Duration::from_secs(30)))
.timeout_recv_body(Some(opts.chunk_timeout))
Author
Collaborator

[low] timeout_recv_body is a total budget for the body (60 s by default), sized for a 512 KiB chunk. When a mirror answers a Range or If-Range request with 200 (CrossWire's ftpmirror redirects to mirrors whose ETag and Last-Modified differ), accept_whole restarts and reads the whole file under that same budget. A rawzip of a few tens of MB on a slow mobile link times out, becomes Step::Retry, and the next attempt may get 200 again and restart from 0. The download never completes. Give the whole-file path a longer or progress-based timeout, or cap the restarts.

[low] timeout_recv_body is a total budget for the body (60 s by default), sized for a 512 KiB chunk. When a mirror answers a Range or If-Range request with 200 (CrossWire's ftpmirror redirects to mirrors whose ETag and Last-Modified differ), accept_whole restarts and reads the whole file under that same budget. A rawzip of a few tens of MB on a slow mobile link times out, becomes Step::Retry, and the next attempt may get 200 again and restart from 0. The download never completes. Give the whole-file path a longer or progress-based timeout, or cap the restarts.
@ -0,0 +62,4 @@
for i in 0..zip.len() {
let mut entry = zip.by_index(i).map_err(|e| bad(e.to_string()))?;
let rel = entry.enclosed_name().ok_or_else(|| bad(format!("unsafe path {:?}", entry.name())))?;
if entry.is_dir() {
Author
Collaborator

[low] unpack copies each zip entry with io::copy and no limit on decompressed size, total size or entry count. A zip bomb (a mirror compromise or a bad package) of a few KB can fill the phone's flash; the ENOSPC can break other app writes such as save-state before the staging folder is cleaned. Bound the total extracted bytes and the entry count.

[low] unpack copies each zip entry with io::copy and no limit on decompressed size, total size or entry count. A zip bomb (a mirror compromise or a bad package) of a few KB can fill the phone's flash; the ENOSPC can break other app writes such as save-state before the staging folder is cleaned. Bound the total extracted bytes and the entry count.
@ -0,0 +207,4 @@
Ok(DataPath::Prefix { dir: parts.iter().collect(), stem })
}
/// Whether `file` (relative to the SWORD folder) is under this path.
Author
Collaborator

[low] A conf whose DataPath has no trailing slash and exactly four components (for example ./modules/texts/ztext/kjv) parses as Prefix{dir: modules/texts/ztext, stem: kjv}. owns() then rejects the module's real files under ztext/kjv/ as outside the DataPath, and remove_data would call fs::remove_file on a directory in the shared driver folder and fail after the conf is already deleted, leaving the data orphaned. Treat a text-driver DataPath as Dir.

[low] A conf whose DataPath has no trailing slash and exactly four components (for example ./modules/texts/ztext/kjv) parses as Prefix{dir: modules/texts/ztext, stem: kjv}. owns() then rejects the module's real files under ztext/kjv/ as outside the DataPath, and remove_data would call fs::remove_file on a directory in the shared driver folder and fail after the conf is already deleted, leaving the data orphaned. Treat a text-driver DataPath as Dir.
core, sword, ci: serialise migrations, pause on a refused start, bound unpacking
All checks were successful
CI / rust (pull_request) Successful in 3m24s
Presets / preset-check (pull_request) Successful in 1m41s
CI / audit (pull_request) Successful in 9s
CI / app (pull_request) Successful in 1m24s
CI / click (pull_request) Successful in 1m16s
8bd9a64b74
- db: one open at a time through backup and migration, each step under
  BEGIN IMMEDIATE with the version re-read, so the reader and the task
  runner can't both migrate a v1 file.
- tasks: a refused start_task or a failed finish halts the queue instead
  of spinning; a cancel arriving with the last progress installs nothing;
  a runner whose database won't open answers every command with an error.
- install: a whole-file 200 gets a body budget sized for the file; unpack
  is limited in bytes and entries; a text-driver DataPath without a
  trailing slash is a folder.
- presets.yml opens an issue when the weekly run fails.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
Collaborator

Answers to the fresh-context review of 7950adaf, fixed in 8bd9a64b. Gates: cargo make green (fmt, clippy, tests with coverage, release, deny, audit), cargo make presets ok (5 sets, 23 modules).

  1. db.rs concurrent migration (medium): confirmed. A new test, concurrent_opens_of_an_old_file_all_migrate (four threads on a v1 file), failed first. Fix: a process-wide lock around version check, backup and migrate, plus each step under BEGIN IMMEDIATE with user_version re-read (covers another process). Passes.
  2. exec.rs start_task hot loop (medium): confirmed. The new test a_database_that_refuses_the_start_pauses_the_queue_instead_of_spinning (trigger aborting UPDATE on tasks) hung on the old code. Fix: a refused start_task, or a failed finish/fail/requeue, halts the queue like the next_task path; one Error, then Paused.
  3. http.rs whole-file timeout (low): confirmed. A 200 to a Range request with a Content-Length above the chunk size is re-requested plainly with a body budget of max(chunk_timeout, total at 8 KiB/s). Costs one extra request on that path; tests a_whole_file_gets_a_budget_for_its_size and the updated no-ranges test.
  4. Cancel with the final progress (low): confirmed. a_cancel_that_arrives_with_the_last_progress_installs_nothing fails on the old code. install() now returns Cancelled before package::install if the flag is set.
  5. Db::open failure leaves a dead runner (low): confirmed. The thread now stays up and answers each command with an Error carrying its number (a_queue_that_cannot_open_its_database_answers_every_command).
  6. Zip bomb (low): confirmed. unpack is limited to 1 GiB extracted and 20,000 entries, reading at most the remaining budget per entry whatever it declares (a_package_that_unpacks_past_the_limits_is_refused).
  7. DataPath without trailing slash (low): confirmed (the new test failed with "outside the module's DataPath"). DataPath::for_conf reads the path as a folder unless ModDrv is a lexicon driver (RawLD, zLD, RawLD4) or absent. Test a_text_module_whose_data_path_has_no_trailing_slash_installs_and_removes.
  8. Silent weekly failure (low): confirmed. presets.yml now opens an issue ("Starter-set presets no longer match CrossWire") when the scheduled run fails, or comments on the open one; docs/09 says so. Not exercised locally (needs a failing scheduled run and the job token's issue write access); if the token lacks it, the step is non-fatal and the failed run on the Actions page remains the record.
Answers to the fresh-context review of 7950adaf, fixed in 8bd9a64b. Gates: `cargo make` green (fmt, clippy, tests with coverage, release, deny, audit), `cargo make presets` ok (5 sets, 23 modules). 1. db.rs concurrent migration (medium): confirmed. A new test, `concurrent_opens_of_an_old_file_all_migrate` (four threads on a v1 file), failed first. Fix: a process-wide lock around version check, backup and migrate, plus each step under BEGIN IMMEDIATE with user_version re-read (covers another process). Passes. 2. exec.rs start_task hot loop (medium): confirmed. The new test `a_database_that_refuses_the_start_pauses_the_queue_instead_of_spinning` (trigger aborting UPDATE on tasks) hung on the old code. Fix: a refused start_task, or a failed finish/fail/requeue, halts the queue like the next_task path; one Error, then Paused. 3. http.rs whole-file timeout (low): confirmed. A 200 to a Range request with a Content-Length above the chunk size is re-requested plainly with a body budget of max(chunk_timeout, total at 8 KiB/s). Costs one extra request on that path; tests `a_whole_file_gets_a_budget_for_its_size` and the updated no-ranges test. 4. Cancel with the final progress (low): confirmed. `a_cancel_that_arrives_with_the_last_progress_installs_nothing` fails on the old code. install() now returns Cancelled before package::install if the flag is set. 5. Db::open failure leaves a dead runner (low): confirmed. The thread now stays up and answers each command with an Error carrying its number (`a_queue_that_cannot_open_its_database_answers_every_command`). 6. Zip bomb (low): confirmed. unpack is limited to 1 GiB extracted and 20,000 entries, reading at most the remaining budget per entry whatever it declares (`a_package_that_unpacks_past_the_limits_is_refused`). 7. DataPath without trailing slash (low): confirmed (the new test failed with "outside the module's DataPath"). DataPath::for_conf reads the path as a folder unless ModDrv is a lexicon driver (RawLD, zLD, RawLD4) or absent. Test `a_text_module_whose_data_path_has_no_trailing_slash_installs_and_removes`. 8. Silent weekly failure (low): confirmed. presets.yml now opens an issue ("Starter-set presets no longer match CrossWire") when the scheduled run fails, or comments on the open one; docs/09 says so. Not exercised locally (needs a failing scheduled run and the job token's issue write access); if the token lacks it, the step is non-fatal and the failed run on the Actions page remains the record.
claude-bot left a comment

Fresh-context review (posted by the author account)

Reviewed 8bd9a64. One low finding, inline below.

Fresh-context review (posted by the author account) Reviewed 8bd9a64. One low finding, inline below.
@ -0,0 +64,4 @@
if echo "$open" | grep -q "\"title\":\"$title\""; then
id=$(echo "$open" | grep -o '"number":[0-9]*' | head -1 | cut -d: -f2)
body="{\"body\":\"Still failing: $RUN\"}"
curl -fsS -X POST -H "Authorization: token $TOKEN" -H "Content-Type: application/json" -d "$body" "$API/issues/$id/comments" || true
Author
Collaborator

low: the dedupe step takes the first "number": from the search response, not the one belonging to the matching title. If q=Starter-set+presets also returns another open issue, grep -q matches the right title but head -1 picks the other issue's number, so the 'Still failing' comment lands on the wrong issue. Failures hidden by || true are accepted (the comment says so).

low: the dedupe step takes the first "number": from the search response, not the one belonging to the matching title. If q=Starter-set+presets also returns another open issue, grep -q matches the right title but head -1 picks the other issue's number, so the 'Still failing' comment lands on the wrong issue. Failures hidden by `|| true` are accepted (the comment says so).
ci: comment on the matching issue, not the first one found
All checks were successful
CI / rust (pull_request) Successful in 3m26s
Presets / preset-check (pull_request) Successful in 1m42s
CI / audit (pull_request) Successful in 8s
CI / app (pull_request) Successful in 1m28s
CI / click (pull_request) Successful in 54s
9aa196a8ae
The weekly presets failure step took the first issue number in the search
response; it now pairs each number with its title and picks the match.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-bot left a comment

Answer to the presets.yml:67 finding: fixed in 9aa196a. Confirmed real (old logic returned 7 for a two-issue response where the match was 12). The step now pairs each "number" with the next "title" and takes the number of the title that matches; checked against a sample response (returns 12) and the YAML still parses. cargo make passes.

Answer to the presets.yml:67 finding: fixed in 9aa196a. Confirmed real (old logic returned 7 for a two-issue response where the match was 12). The step now pairs each "number" with the next "title" and takes the number of the title that matches; checked against a sample response (returns 12) and the YAML still parses. cargo make passes.
claude-review left a comment

Coverage review (medium)

Range: origin/main...origin/phase1/starter-sets-core at 9aa196a, plus the commit range 8bd9a64..9aa196a (presets.yml issue-dedupe fix).

Files reviewed:

  • sword/src/install/testserver.rs
  • sword/src/install/http/tests.rs
  • sword/src/install/index/tests.rs
  • sword/src/install/package/tests.rs
  • core/src/db/tasks/tests.rs
  • core/src/tasks/tests.rs
  • qt/Cargo.lock
  • Cargo.lock
  • deny.toml (CDLA exception)
  • .forgejo/workflows/presets.yml (8bd9a64..9aa196a)
    (read for context: sword/src/install/{http,index,package}.rs, sword/src/install.rs, core/src/tasks.rs, core/src/tasks/exec.rs, core/src/db/tasks.rs, 002_tasks.sql, sword/Cargo.toml, core/Cargo.toml)

Result: no high or medium findings. The tests passed 3 runs in a row under parallel cargo test (sword install: 36 tests; core tasks and db::tasks: 26 tests), touch only 127.0.0.1, and cargo deny --offline check licenses bans is ok. The new crates (ureq 3.5, rustls 0.23, ring, rustls-webpki, webpki-roots, zip 9, toml 0.9, serde, typed-path, zeroize, subtle) are all MIT, Apache-2.0, ISC or BSD, plus CDLA-Permissive-2.0 for webpki-roots only (the crate-scoped exception is correct). None is GPL-2.0-only. The presets.yml dedupe fix is correct: "number" is 9 characters so substr($0,10) is right, and the issue's own title comes before the label and milestone titles in Forgejo's JSON.

Low findings:

  1. low · core/src/tasks/tests.rs:373-390 · a_cancel_that_arrives_with_the_last_progress_installs_nothing does not reach the branch it names. The Cancel is already queued, so Ctx::on_download reads it on the first download event, which is Event::Start, not the last progress. The stop flag is then set before any byte is fetched, download returns Err(Cancelled) and the task ends as Cancelled through exec.rs:459. The guard at core/src/tasks/exec.rs:462 (Ok(()) with cancelled set) is never executed. Deleting that guard keeps every test green, and a real cancel that lands on the final Progress or Downloaded event would then install the module and report Done after the user cancelled. Fix: send the Cancel from inside the event callback only when the event is Downloaded (or the final Progress).

  2. low · core/src/tasks/exec.rs:409-411 · The Done branch of finish() emits Event::Done before db.finish_task, while the Failed branch (exec.rs:415-416) writes the row first. A listener that reloads the task list on Done can read the row as still 'running'. The tests only look at the table after Idle, so they cannot see it. Fix: write the row first in the Done branch, and add a test that reads db.tasks() inside the callback on Done.

  3. low · sword/src/install/package.rs:213-219 · DataPath::for_conf treats every driver except RawLD/zLD/RawLD4 as a folder. SWORD's RawGenBook keeps DataPath as a file-name prefix (./modules/genbook/rawgenbook/x/x with x.bdt, x.bdx, ...). For a genbook, check() would refuse the install with "outside the module's DataPath". The tests cover only zText and a prefix-style lexicon. No current preset has a genbook, so nothing breaks today, but the module manager will offer any module in the index.

  4. low · sword/src/install/testserver.rs:127-131 and 135-169 · The test server differs from real servers in two ways that hide client behaviour. (a) It answers a Range that starts past the end with 200 and the whole file, where Apache answers 416 (parse_range returns None when a > b). The client's 416 path (http.rs:338, Fatal Http(416)) is never exercised. (b) It never redirects. CrossWire's /ftpmirror/ URL redirects to mirrors, ureq follows the redirect and re-sends Range and If-Range to a host with a different ETag or Last-Modified. The result is a 200 and a restart (http.rs:372-392) that no test models. Fix: add a redirect knob to the server and a test that resumes against a second origin with another ETag.

  5. low · sword/src/install/http/tests.rs:90-102 (target: http.rs:137, 361-375, 218-223) · Three resume branches have no test. (a) The Last-Modified fallback of State::validator, because the server never sends Last-Modified or weak ETags. (b) accept_range's "size changed" restart, the "server ignores If-Range" case, because the server always honours If-Range, so a_changed_file_restarts only covers the 200 path. (c) The "part longer than total" restart at http.rs:137. A regression there would write mixed-version bytes into the zip, and only the zip CRC would catch it.

  6. low · sword/src/install/package/tests.rs:451-455 · a_package_that_unpacks_past_the_limits_is_refused does not prove the budget is cumulative. CONF is 51 bytes, so with bytes: 50 the first entry already trips the limit. Code that checked each entry against the limit separately would pass. Fix: use two entries of 30 bytes with bytes: 50.

  7. low · sword/src/install/testserver.rs:46-53 · Server has no Drop. Each Server::start leaves its accept thread blocked and the listening socket open until the test process exits. Pooled keep-alive connection threads also live until the client agent closes. With about 60 tests it is harmless, but the preset check and core tests will keep adding to it. Fix: add a Drop that sets a flag and connects once to wake the accept loop.

  8. low · .forgejo/workflows/presets.yml:13-19 · The check also runs on every PR that touches sword/src/install/, and it compares against CrossWire's live index. A CrossWire outage (exit 2) or an unrelated change in a module's size or licence turns an unrelated installer PR red. Consider keeping the PR run for presets/ and preset-check/** only, and leaving the install code to the weekly run and the local-server tests.

  9. low · Cargo.lock and qt/Cargo.lock (winnow 0.7.15 and 1.0.4) · toml 0.9 brings in two versions of winnow (cargo deny warns on duplicates), and the release app now links serde, toml, ureq, rustls, ring and webpki-roots. Nothing in the PR states the effect on the click size budget (docs/07: 15 MB) or on start-up and memory, which the repo CLAUDE.md asks of a PR that adds them. ring also needs a C and asm toolchain for the arm64 build, and the Cargo.toml comment says so, but the PR gives no measured click size.

Coverage review (medium) Range: origin/main...origin/phase1/starter-sets-core at 9aa196a, plus the commit range 8bd9a64..9aa196a (presets.yml issue-dedupe fix). Files reviewed: - sword/src/install/testserver.rs - sword/src/install/http/tests.rs - sword/src/install/index/tests.rs - sword/src/install/package/tests.rs - core/src/db/tasks/tests.rs - core/src/tasks/tests.rs - qt/Cargo.lock - Cargo.lock - deny.toml (CDLA exception) - .forgejo/workflows/presets.yml (8bd9a64..9aa196a) (read for context: sword/src/install/{http,index,package}.rs, sword/src/install.rs, core/src/tasks.rs, core/src/tasks/exec.rs, core/src/db/tasks.rs, 002_tasks.sql, sword/Cargo.toml, core/Cargo.toml) Result: no high or medium findings. The tests passed 3 runs in a row under parallel cargo test (sword install: 36 tests; core tasks and db::tasks: 26 tests), touch only 127.0.0.1, and `cargo deny --offline check licenses bans` is ok. The new crates (ureq 3.5, rustls 0.23, ring, rustls-webpki, webpki-roots, zip 9, toml 0.9, serde, typed-path, zeroize, subtle) are all MIT, Apache-2.0, ISC or BSD, plus CDLA-Permissive-2.0 for webpki-roots only (the crate-scoped exception is correct). None is GPL-2.0-only. The presets.yml dedupe fix is correct: "number" is 9 characters so substr($0,10) is right, and the issue's own title comes before the label and milestone titles in Forgejo's JSON. Low findings: 1. low · core/src/tasks/tests.rs:373-390 · a_cancel_that_arrives_with_the_last_progress_installs_nothing does not reach the branch it names. The Cancel is already queued, so Ctx::on_download reads it on the first download event, which is Event::Start, not the last progress. The stop flag is then set before any byte is fetched, download returns Err(Cancelled) and the task ends as Cancelled through exec.rs:459. The guard at core/src/tasks/exec.rs:462 (Ok(()) with cancelled set) is never executed. Deleting that guard keeps every test green, and a real cancel that lands on the final Progress or Downloaded event would then install the module and report Done after the user cancelled. Fix: send the Cancel from inside the event callback only when the event is Downloaded (or the final Progress). 2. low · core/src/tasks/exec.rs:409-411 · The Done branch of finish() emits Event::Done before db.finish_task, while the Failed branch (exec.rs:415-416) writes the row first. A listener that reloads the task list on Done can read the row as still 'running'. The tests only look at the table after Idle, so they cannot see it. Fix: write the row first in the Done branch, and add a test that reads db.tasks() inside the callback on Done. 3. low · sword/src/install/package.rs:213-219 · DataPath::for_conf treats every driver except RawLD/zLD/RawLD4 as a folder. SWORD's RawGenBook keeps DataPath as a file-name prefix (./modules/genbook/rawgenbook/x/x with x.bdt, x.bdx, ...). For a genbook, check() would refuse the install with "outside the module's DataPath". The tests cover only zText and a prefix-style lexicon. No current preset has a genbook, so nothing breaks today, but the module manager will offer any module in the index. 4. low · sword/src/install/testserver.rs:127-131 and 135-169 · The test server differs from real servers in two ways that hide client behaviour. (a) It answers a Range that starts past the end with 200 and the whole file, where Apache answers 416 (parse_range returns None when a > b). The client's 416 path (http.rs:338, Fatal Http(416)) is never exercised. (b) It never redirects. CrossWire's /ftpmirror/ URL redirects to mirrors, ureq follows the redirect and re-sends Range and If-Range to a host with a different ETag or Last-Modified. The result is a 200 and a restart (http.rs:372-392) that no test models. Fix: add a redirect knob to the server and a test that resumes against a second origin with another ETag. 5. low · sword/src/install/http/tests.rs:90-102 (target: http.rs:137, 361-375, 218-223) · Three resume branches have no test. (a) The Last-Modified fallback of State::validator, because the server never sends Last-Modified or weak ETags. (b) accept_range's "size changed" restart, the "server ignores If-Range" case, because the server always honours If-Range, so a_changed_file_restarts only covers the 200 path. (c) The "part longer than total" restart at http.rs:137. A regression there would write mixed-version bytes into the zip, and only the zip CRC would catch it. 6. low · sword/src/install/package/tests.rs:451-455 · a_package_that_unpacks_past_the_limits_is_refused does not prove the budget is cumulative. CONF is 51 bytes, so with bytes: 50 the first entry already trips the limit. Code that checked each entry against the limit separately would pass. Fix: use two entries of 30 bytes with bytes: 50. 7. low · sword/src/install/testserver.rs:46-53 · Server has no Drop. Each Server::start leaves its accept thread blocked and the listening socket open until the test process exits. Pooled keep-alive connection threads also live until the client agent closes. With about 60 tests it is harmless, but the preset check and core tests will keep adding to it. Fix: add a Drop that sets a flag and connects once to wake the accept loop. 8. low · .forgejo/workflows/presets.yml:13-19 · The check also runs on every PR that touches sword/src/install/**, and it compares against CrossWire's live index. A CrossWire outage (exit 2) or an unrelated change in a module's size or licence turns an unrelated installer PR red. Consider keeping the PR run for presets/** and preset-check/** only, and leaving the install code to the weekly run and the local-server tests. 9. low · Cargo.lock and qt/Cargo.lock (winnow 0.7.15 and 1.0.4) · toml 0.9 brings in two versions of winnow (cargo deny warns on duplicates), and the release app now links serde, toml, ureq, rustls, ring and webpki-roots. Nothing in the PR states the effect on the click size budget (docs/07: 15 MB) or on start-up and memory, which the repo CLAUDE.md asks of a PR that adds them. ring also needs a C and asm toolchain for the arm64 build, and the Cargo.toml comment says so, but the PR gives no measured click size.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
install: tests for the Last-Modified fallback, the size-changed restart and a part longer than its total
All checks were successful
CI / rust (pull_request) Successful in 4m0s
Presets / preset-check (pull_request) Successful in 1m58s
CI / audit (pull_request) Successful in 11s
CI / app (pull_request) Successful in 1m43s
CI / click (pull_request) Successful in 1m7s
ce21b29911
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge origin/main (picker and history) into phase1/starter-sets-core
Some checks failed
CI / rust (pull_request) Successful in 3m51s
Presets / preset-check (pull_request) Failing after 2m0s
CI / audit (pull_request) Successful in 10s
CI / app (pull_request) Successful in 2m23s
CI / click (pull_request) Successful in 1m17s
3b616f1af3
core/src/db.rs: keep both the history and tasks modules.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-review left a comment

Delta review (medium), 9aa196a..3b616f1

Files reviewed:

  • core/src/tasks/exec.rs
  • core/src/tasks/tests.rs
  • core/src/db.rs (merge 3b616f1 vs ce21b29), core/src/db/history.rs, core/src/db/001_init.sql, core/src/db/002_tasks.sql
  • core/src/worker.rs (Event wiring)
  • sword/src/install/package.rs
  • sword/src/install/package/tests.rs
  • sword/src/install/http/tests.rs
  • sword/src/install/http.rs
  • sword/src/install/testserver.rs

Findings: no high or medium findings.

  • finish_task before Event::Done: correct. A failed save still emits Done and then Error and halt, as it did before; this is unchanged behaviour, not a new defect.
  • Cancel test: the cancel is sent from the listener on the last Progress, so only that event's drain reads it. It reaches the Ok(()) if cancelled guard in install(). Removing the guard makes it fail (Done would be seen).
  • RawGenBook DataPath as a file prefix: the fix and its install/remove test are consistent.
  • Merge 3b616f1: only adds mod history; and pub use history::HISTORY_CAP; beside mod tasks. The migration list is unchanged (001 already holds the history table, 002 is tasks). tasks::Event and worker::Event are separate types, so there is no variant clash and no shared wiring.
  • Workspace tests (excluding utsb-qt) pass locally at 3b616f1; core lib 98 passed.

Low:

  • low · sword/src/install/package.rs:217 · d == "RawGenBook" is an exact match, while the LD checks are suffix matches. A conf with different ModDrv casing or spacing would take the Dir branch and mis-resolve the prefix. This is only a hypothetical for odd modules.
Delta review (medium), 9aa196a..3b616f1 Files reviewed: - core/src/tasks/exec.rs - core/src/tasks/tests.rs - core/src/db.rs (merge 3b616f1 vs ce21b29), core/src/db/history.rs, core/src/db/001_init.sql, core/src/db/002_tasks.sql - core/src/worker.rs (Event wiring) - sword/src/install/package.rs - sword/src/install/package/tests.rs - sword/src/install/http/tests.rs - sword/src/install/http.rs - sword/src/install/testserver.rs Findings: no high or medium findings. - finish_task before Event::Done: correct. A failed save still emits Done and then Error and halt, as it did before; this is unchanged behaviour, not a new defect. - Cancel test: the cancel is sent from the listener on the last Progress, so only that event's drain reads it. It reaches the `Ok(()) if cancelled` guard in install(). Removing the guard makes it fail (Done would be seen). - RawGenBook DataPath as a file prefix: the fix and its install/remove test are consistent. - Merge 3b616f1: only adds `mod history;` and `pub use history::HISTORY_CAP;` beside `mod tasks`. The migration list is unchanged (001 already holds the history table, 002 is tasks). tasks::Event and worker::Event are separate types, so there is no variant clash and no shared wiring. - Workspace tests (excluding utsb-qt) pass locally at 3b616f1; core lib 98 passed. Low: - low · sword/src/install/package.rs:217 · `d == "RawGenBook"` is an exact match, while the LD checks are suffix matches. A conf with different ModDrv casing or spacing would take the Dir branch and mis-resolve the prefix. This is only a hypothetical for odd modules.
ci: the preset check asks a HEAD again when it gets no answer, and one that never does is "could not check"
Some checks failed
CI / audit (pull_request) Has been cancelled
CI / app (pull_request) Has been cancelled
CI / click (pull_request) Has been cancelled
Presets / preset-check (pull_request) Has been cancelled
CI / rust (pull_request) Has been cancelled
bbd9939655
CI on 3b616f1 reported StrongsGreek as having no rawzip because one HEAD
got 'Peer disconnected'. A dropped connection says nothing about the
module, so it is retried 4 times, 3 s apart; if it never answers, the
check exits 2 (could not run) instead of 1 (a module changed). A status
such as 404 is still a problem, and is not asked again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-review left a comment

Delta review (3b616f1..bbd9939, medium)

Checked: retry count (4 tries, 3 s apart, 3 sleeps), only Error::Network retries (Http/Protocol pass straight through; http_status_as_error(false) in sword/src/install/http.rs:50 means a 404 really arrives as Error::Http, so it stays a Problem), the RefCell bookkeeping (first failure kept, no overlapping borrow), and main.rs mapping run()'s Err to exit 2. cargo test -p utsb-preset-check passes.

low — tools/ci/preset-check/src/lib.rs:175 — after a module's HEAD has finally failed with Network, check() carries on and retries every remaining module — failure scenario: CrossWire's package host is down or blackholed (connect timeout 15 s): each module costs 4 x 15 s + 3 x 3 s = about 69 s, so 23 modules hold the single runner for about 26 min (about 3.5 min of that is pauses even with instant refusals) before reporting exit 2. Short-circuit: once unreachable is set, return Err immediately for later modules without calling head.

low — tools/ci/preset-check/src/tests.rs:177 — the tests do not count attempts, so "4 tries" is not pinned: a_head_that_gets_no_answer_once passes with 2 tries and the never-answers test passes with 1 or 100 — failure scenario: someone changes HEAD_TRIES or the guard tries < HEAD_TRIES and nothing fails. Have heads() expose its call count and assert 4 for the persistent case and 1 for the 404 case (the 404 test's second answer Ok(500) only shows the 404 wins, not that it was not re-asked).

low — tools/ci/preset-check/src/lib.rs:182 — a persistent network failure turns the whole result into Err, which drops Problems already found for other modules (e.g. a real licence change) — failure scenario: one real change plus one flaky HEAD gives exit 2 "could not check" with no mention of the change. Acceptable by design, but consider printing the collected problems alongside the error.

low — tools/ci/preset-check/tests/cli.rs — no CLI test for the new path (persistent Network on a package HEAD gives exit 2 and not 1); run()'s wiring of the closure into check_live is only covered by reading — failure scenario: a regression in the run() to check_live hookup would go unnoticed.

Delta review (3b616f1..bbd9939, medium) Checked: retry count (4 tries, 3 s apart, 3 sleeps), only Error::Network retries (Http/Protocol pass straight through; http_status_as_error(false) in sword/src/install/http.rs:50 means a 404 really arrives as Error::Http, so it stays a Problem), the RefCell bookkeeping (first failure kept, no overlapping borrow), and main.rs mapping run()'s Err to exit 2. cargo test -p utsb-preset-check passes. low — tools/ci/preset-check/src/lib.rs:175 — after a module's HEAD has finally failed with Network, check() carries on and retries every remaining module — failure scenario: CrossWire's package host is down or blackholed (connect timeout 15 s): each module costs 4 x 15 s + 3 x 3 s = about 69 s, so 23 modules hold the single runner for about 26 min (about 3.5 min of that is pauses even with instant refusals) before reporting exit 2. Short-circuit: once `unreachable` is set, return Err immediately for later modules without calling head. low — tools/ci/preset-check/src/tests.rs:177 — the tests do not count attempts, so "4 tries" is not pinned: a_head_that_gets_no_answer_once passes with 2 tries and the never-answers test passes with 1 or 100 — failure scenario: someone changes HEAD_TRIES or the guard `tries < HEAD_TRIES` and nothing fails. Have heads() expose its call count and assert 4 for the persistent case and 1 for the 404 case (the 404 test's second answer Ok(500) only shows the 404 wins, not that it was not re-asked). low — tools/ci/preset-check/src/lib.rs:182 — a persistent network failure turns the whole result into Err, which drops Problems already found for other modules (e.g. a real licence change) — failure scenario: one real change plus one flaky HEAD gives exit 2 "could not check" with no mention of the change. Acceptable by design, but consider printing the collected problems alongside the error. low — tools/ci/preset-check/tests/cli.rs — no CLI test for the new path (persistent Network on a package HEAD gives exit 2 and not 1); run()'s wiring of the closure into check_live is only covered by reading — failure scenario: a regression in the run() to check_live hookup would go unnoticed.
ci: after a package host gives no answer, the preset check asks no further modules
All checks were successful
CI / rust (pull_request) Successful in 4m0s
Presets / preset-check (pull_request) Successful in 2m9s
CI / audit (pull_request) Successful in 12s
CI / app (pull_request) Successful in 2m17s
CI / click (pull_request) Successful in 1m16s
c18f37ecc9
Review 214: a blackholed host cost every module four 15 s timeouts and
three pauses, about 26 min on the one runner. The tests now count how
often the HEAD was asked (2 after one drop, 4 when it never answers,
1 for a 404).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-review left a comment

Delta review (bbd9939..c18f37e, medium)

No findings.

Checked: the early-return unreachable.borrow() guard is dropped at the return, so the later borrow_mut() cannot panic. get_or_insert_with keeps the first module's failure as the message. Skipped modules return "not asked, as ..." and check() turns that into NoPackage problems, but check_live discards them with the Err (the accepted note 3). Tests pass: cargo test -q -p utsb-preset-check ran 4 tests, all passing. asked.get() == HEAD_TRIES with two modules shows the second module is never asked. The other tests count 2 and 1 HEAD calls.

Delta review (bbd9939..c18f37e, medium) No findings. Checked: the early-return `unreachable.borrow()` guard is dropped at the return, so the later `borrow_mut()` cannot panic. `get_or_insert_with` keeps the first module's failure as the message. Skipped modules return "not asked, as ..." and `check()` turns that into NoPackage problems, but `check_live` discards them with the Err (the accepted note 3). Tests pass: `cargo test -q -p utsb-preset-check` ran 4 tests, all passing. `asked.get() == HEAD_TRIES` with two modules shows the second module is never asked. The other tests count 2 and 1 HEAD calls.
chore: record review result
All checks were successful
CI / rust (pull_request) Successful in 4m12s
Presets / preset-check (pull_request) Successful in 2m18s
CI / audit (pull_request) Successful in 12s
CI / app (pull_request) Successful in 2m21s
CI / click (pull_request) Successful in 1m16s
c5e1cd8518
Review-Confidence: 55
Review-Tier: trunk
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-bot changed title from WIP: F1 starter sets 1/2: presets, resumable install tasks, preset CI check to F1 starter sets 1/2: presets, resumable install tasks, preset CI check 2026-10-22 08:06:37 +00:00
claude-review left a comment

Merge gate (step 8)

  1. High/medium findings: PASS. No highs. The two mediums from r1 (7950ada) are fixed in 8bd9a64 and present at head: migration race (process-wide lock, each step under BEGIN IMMEDIATE with user_version re-read in db.rs:187-201, test concurrent_opens_of_an_old_file_all_migrate, failed first) and the start_task hot loop (exec.rs halt() on a refused start_task or failed finish/fail/requeue, test hung on old code). The r2 re-review of 8bd9a64 found one low only, fixed in 9aa196a.
  2. Commits after the last full round (9aa196a..HEAD): PASS. Read the list and diffs. ce21b29 and ba394fd (cancel test, finish before Done, cumulative budget, three resume tests), 7d99b6b (RawGenBook prefix) are covered by reviews 209 and 213. bbd9939 and c18f37e (tools/ci/preset-check/src/lib.rs, tests.rs only) are covered by 214 and 215, and I re-read the lib.rs diff: retry only on Error::Network, status stays a Problem, later modules skipped once unreachable, no RefCell overlap. 3b616f1 is a merge of main (#27) that adds only history wiring. c5e1cd8 is an empty trailer commit (Review-Confidence 55, Review-Tier trunk). Deltas 213-215 raised only lows: 1 declined with reason, 2 fixed, 2 deferred to #28 and #29 (416 path, preset-check exit 2 drops found problems; CLI test).
  3. CI on c18f37e: PASS. Commit status success, all 5 contexts (CI / rust, CI / audit, CI / app, CI / click, Presets / preset-check).
  4. Testing-tier gates (maximum + elevated): PASS, none unrun, none AI-behavior. Maximum: standard run via cargo make app-docker mem-check in the production-like clickable image (PSS 147478 kB under 165000, kill -9 restore ok) on 3b616f1; the later commits touch only tools/ci/preset-check. Targeted checks are named, with mutants shown failing: cargo-deny and cargo-audit on the new deps, and the preset-check job (green live in CI, exit 1 on a broken copy). Elevated: targets written down (CrossWire GET/HEAD only; throwaway DBs for migration 002; local mock API for the issue step), and the effect was read back (live "ok: 5 sets, 23 modules", migration test, mock-issue outcomes). The body lists the gaps honestly under Not covered (no arm64 run, click size unmeasured, 416 path) and none is a tier gate. Both are nothing-wired-yet items for part 2.

Review-Confidence is 55; that is the author's score and is not what this gate checks. Approving head c5e1cd8.

Merge gate (step 8) 1. High/medium findings: PASS. No highs. The two mediums from r1 (7950ada) are fixed in 8bd9a64 and present at head: migration race (process-wide lock, each step under BEGIN IMMEDIATE with user_version re-read in db.rs:187-201, test concurrent_opens_of_an_old_file_all_migrate, failed first) and the start_task hot loop (exec.rs halt() on a refused start_task or failed finish/fail/requeue, test hung on old code). The r2 re-review of 8bd9a64 found one low only, fixed in 9aa196a. 2. Commits after the last full round (9aa196a..HEAD): PASS. Read the list and diffs. ce21b29 and ba394fd (cancel test, finish before Done, cumulative budget, three resume tests), 7d99b6b (RawGenBook prefix) are covered by reviews 209 and 213. bbd9939 and c18f37e (tools/ci/preset-check/src/lib.rs, tests.rs only) are covered by 214 and 215, and I re-read the lib.rs diff: retry only on Error::Network, status stays a Problem, later modules skipped once unreachable, no RefCell overlap. 3b616f1 is a merge of main (#27) that adds only history wiring. c5e1cd8 is an empty trailer commit (Review-Confidence 55, Review-Tier trunk). Deltas 213-215 raised only lows: 1 declined with reason, 2 fixed, 2 deferred to #28 and #29 (416 path, preset-check exit 2 drops found problems; CLI test). 3. CI on c18f37e: PASS. Commit status success, all 5 contexts (CI / rust, CI / audit, CI / app, CI / click, Presets / preset-check). 4. Testing-tier gates (maximum + elevated): PASS, none unrun, none AI-behavior. Maximum: standard run via cargo make app-docker mem-check in the production-like clickable image (PSS 147478 kB under 165000, kill -9 restore ok) on 3b616f1; the later commits touch only tools/ci/preset-check. Targeted checks are named, with mutants shown failing: cargo-deny and cargo-audit on the new deps, and the preset-check job (green live in CI, exit 1 on a broken copy). Elevated: targets written down (CrossWire GET/HEAD only; throwaway DBs for migration 002; local mock API for the issue step), and the effect was read back (live "ok: 5 sets, 23 modules", migration test, mock-issue outcomes). The body lists the gaps honestly under Not covered (no arm64 run, click size unmeasured, 416 path) and none is a tier gate. Both are nothing-wired-yet items for part 2. Review-Confidence is 55; that is the author's score and is not what this gate checks. Approving head c5e1cd8.
schaefera deleted branch phase1/starter-sets-core 2026-10-22 08:24:26 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
schaefera/utStudyBIble!26
No description provided.