Expand test coverage for site_generator, importer, downloader, instance (#60) by lmorchard · Pull Request #73 · lmorchard/fossilizer

lmorchard · 2026-07-24T23:54:58Z

Builds a real test safety net around fossilizer's largest side-effecting subsystems, plus the minimal production changes that unblock testing them. Suite grows from 22 → 41 lib tests (43 total incl. integration + doc).

What changed

Test coverage (#60) across site_generator, mastodon/importer, downloader, and mastodon/instance, on top of a new #[cfg(test)] SQLite helper (db::open_in_memory / open_migrated_file).

Production changes (minimal, to unblock the above):

Reduce reliance on global mutable config to enable unit testing #55 — injectable config. Thread &AppConfig through the library functions that previously read the process-global config singleton (db::conn/upgrade, templates::init, site_generator::{setup_data_path, unpack_customizable_resources, copy_web_assets, generate_activit*}, instance::{load,save}_instance_config). The global + config::init() remain only at the CLI boundary; each command fetches config::config() once and passes &config down. Behavior-preserving. config::config() now appears only under src/cli/.
Security: zip-slip path traversal in tar/gz import #49 — zip-slip fix. The tar import path joined archive entry paths directly; Importer::handle_media_attachment now rejects any entry with a ../absolute/prefix component and bails the whole import. (The zip path was already safe via enclosed_name().) Covered by an end-to-end regression test with a hand-crafted ../ tar entry.
Downloader: surface download failures, add timeouts, share client, stream to disk #51 (surface-failures slice only). Downloader::run discarded each worker's JoinHandle result, silently swallowing failed downloads. It now counts failures, keeps draining the queue, and returns an error once drained (happy path unchanged). Timeouts / shared client / streaming-to-disk remain out of scope.

Design decisions

&AppConfig by reference, not Arc or a test-only global setter — it's Sync, so the rayon workers in generate_activities_pages borrow it directly. Simplest change that unblocks isolated, parallel-safe tests. A future AppContext services-struct is the scale-up shape (documented in the session notes), deliberately deferred as over-engineering for now.
Zip-slip → bail the whole import: a crafted traversal entry is hostile, so fail loudly rather than partially extract.

Verification

make check (fmt + clippy -D warnings) and make test green.
TDD where behavior changed: the zip-slip and downloader-surfacing tests were confirmed red before their fixes.
Two independent review passes (the Reduce reliance on global mutable config to enable unit testing #55 refactor and the whole branch) came back clean.
Real init → import → build smoke run produced valid index.html/index.json + per-day pages through the refactored config path.

Deferred (not in this PR)

Rest of #51 (timeouts, shared client, streaming); #57 (remote-actor URL); the AppContext services-struct refactor; the project-wide &String→&str sweep.

Spec, plan, research, and notes: docs/dev-sessions/2026-07-24-1521-expand-test-coverage/.

Closes #60
Closes #49
Closes #55

🤖 Generated with Claude Code

Add a #[cfg(test)] db::open_in_memory() that builds an isolated, fully-migrated in-memory database (rarray vtab + foreign_keys, no WAL pragma since it does not apply to :memory:). Reads no config, so DB-dependent unit tests stay hermetic and parallel-safe. Add tempfile as a dev-dependency for the filesystem tests in later phases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…h tests Downloader::run previously discarded each worker's JoinHandle result, so a failed download was silently swallowed. Count per-task failures (logging each), keep draining the queue, and bail after the loop when any failed — the happy path still returns Ok. Add tests for DownloadTask::execute error paths (non-2xx, unwritable destination) and for run() surfacing a failure. Scope: only the failure-surfacing slice of #51; timeouts, a shared client, and streaming-to-disk remain out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add a path-traversal guard in Importer::handle_media_attachment: reject any entry with a ParentDir/RootDir/Prefix component and bail the whole import. The zip path was already safe via enclosed_name(); this closes the tar path, which joined entry paths directly. Tests (src/mastodon/importer.rs): a hand-crafted raw tar with a `../`-escaping entry is rejected end-to-end through import() (regression for #49); unsupported extensions error; the tar.gz and zip export fixtures ingest their outbox/actor via the by-extension dispatch; skip_media extracts no files. Also add db::open_migrated_file(&Path) test helper (file-backed sibling of open_in_memory) so a test can query a DB after handing its Connection to the Importer, which takes ownership. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Unit tests for the site_generator paths that already take explicit params, using the in-memory DB helper and the activity fixture: plan_activities_pages day-context building and prev/next linking across two days, generate_index_json write + round-trip, and setup_build_path create/clean (incl. NotFound tolerance on a nonexistent path). No production changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

#55) Thread &AppConfig through the functions that read the process-global config singleton, so library code no longer touches it: db::conn/upgrade, templates::init, site_generator::{setup_data_path, unpack_customizable_resources, copy_web_assets, generate_activities_pages, generate_activity_page}, and mastodon::instance::{load,save}_instance_config. The global + config::init() stay as the CLI-boundary source; each command fetches config::config() once and passes &config down. &AppConfig by reference (it's Sync, so the rayon workers in generate_activities_pages borrow it directly — no Arc). Behavior-preserving: verified by the full test suite, tests/exit_code.rs, and a real init/import/build smoke run. config::config() now appears only under src/cli/. build_instance_config_path lost its only fallible step and now returns PathBuf. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

site_generator: render the site index HTML and a per-day activity page via templates::init(&config) + the embedded default theme (the per-day test seeds an on-disk DB since generate_activity_page opens its own connection); copy_web_assets falls back to embedded web assets when no theme dir exists; setup_data_path create/clean. mastodon::instance: save/load round-trip preserves every field, and loading a missing instance returns a fresh default. Suite is now 41 lib tests (from 22 at baseline), covering site_generator, importer, downloader, and instance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Record spec/plan/research/notes for the session. Per the final whole-branch review, strengthen generate_activity_page_renders_day to assert the rendered page is a full HTML document containing the day date, rather than merely non-empty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Expands automated test coverage across fossilizer’s largest side-effecting subsystems (site generation, Mastodon import, media downloading, and instance config persistence) and makes minimal production refactors to make those paths testable and safer (notably config injection, tar-path traversal protection, and surfacing downloader task failures).

Changes:

Thread &AppConfig through previously-global-config-dependent library code (db/templates/site_generator/instance) and update CLI call sites to fetch config once and pass it down.
Add a test-only SQLite helper (db::open_in_memory / db::open_migrated_file) and a large set of new unit tests covering site planning/rendering, importer behavior (incl. zip-slip regression), downloader failure surfacing, and instance config round-trips.
Improve downloader behavior to observe worker results and return an error after draining if any tasks failed; add tempfile for filesystem-based tests.

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`src/templates.rs`	Makes template initialization configurable by injecting `&AppConfig` (removes dependency on global config).
`src/site_generator.rs`	Injects `&AppConfig` for config-dependent operations and adds extensive unit tests for planning, rendering, assets, and data path setup.
`src/mastodon/instance.rs`	Injects `&AppConfig` for instance config persistence and adds round-trip tests.
`src/mastodon/importer.rs`	Adds a tar-path zip-slip/path traversal guard and comprehensive importer tests (fixtures + regression archive).
`src/downloader.rs`	Surfaces worker/task failures from `Downloader::run` and adds tests for error paths and run-loop failure propagation.
`src/db.rs`	Refactors `conn/upgrade` to accept `&AppConfig` and adds `#[cfg(test)]` DB helpers for hermetic, migrated SQLite connections plus tests.
`src/cli/upgrade.rs`	Updates CLI wiring to pass injected config into `db::upgrade`.
`src/cli/mastodon/fetch.rs`	Updates DB connection creation to use injected config.
`src/cli/mastodon.rs`	Fetches config once at the command boundary and passes it into instance config load/save.
`src/cli/init.rs`	Updates init flow to use injected config for data setup, DB upgrade, and optional customization.
`src/cli/import.rs`	Updates importer CLI to open DB via injected config.
`src/cli/fetch.rs`	Updates DB connection creation to use injected config.
`src/cli/build.rs`	Updates build flow to inject config into asset copy, templates init, DB connection, and per-day generation.
`Cargo.toml`	Adds `tempfile` dev-dependency to support filesystem-based tests.
`Cargo.lock`	Locks new dev-dependency (`tempfile` and transitive deps).
`docs/dev-sessions/2026-07-24-1521-expand-test-coverage/spec.md`	Adds spec documenting goals, decisions, and constraints for the test/DI work.
`docs/dev-sessions/2026-07-24-1521-expand-test-coverage/research.md`	Adds research notes mapping config/global usage and test seams.
`docs/dev-sessions/2026-07-24-1521-expand-test-coverage/plan.md`	Adds phased implementation plan and verification checklist.
`docs/dev-sessions/2026-07-24-1521-expand-test-coverage/notes.md`	Adds execution notes, deviations, and verification outcomes for the work.

Comments suppressed due to low confidence (1)

src/site_generator.rs:145

generate_activity_page returns Result<()>, but later in the body it unwrap()s the activity’s actor id and the actor lookup, which can panic during a build. Prefer returning an error so failures are surfaced cleanly to callers.

) -> Result<()> {
    let db_conn = db::conn(config)?;
    let db_activities = db::activities::Activities::new(&db_conn);

    let day = &day_entry.current.date;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

 pub fn generate_activities_pages(
+    config: &AppConfig,
    build_path: &PathBuf,
    tera: &Tera,
    actors: &HashMap<String, activitystreams::Actor>,
    day_entries: &Vec<contexts::IndexDayContext>,
 ) -> Result<()> {
    info!("Generating {} per-day pages", day_entries.len());
-    day_entries
-        .par_iter()
-        .for_each(|day_entry| generate_activity_page(build_path, tera, actors, day_entry).unwrap());
+    day_entries.par_iter().for_each(|day_entry| {
+        generate_activity_page(config, build_path, tera, actors, day_entry).unwrap();
+    });
    Ok(())


+fn build_instance_config_path(config: &AppConfig, instance: &String) -> PathBuf {
    // todo: hash the instance domain string rather than using it verbatim?
-    Ok(data_path.join(format!("config-instance-{instance}.toml")))
+    config
+        .data_path
+        .join(format!("config-instance-{instance}.toml"))


build_instance_config_path embeds the instance name verbatim in the config filename; reject names containing path separators or `..` so a crafted --instance can't escape data_path (same bug class as the tar zip-slip fixed in this PR). Real Mastodon hostnames never contain these characters, so valid names are unaffected. Add a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

lmorchard · 2026-07-25T00:03:55Z

Copilot review triage

Fixed in this PR:

build_instance_config_path path traversal — the instance name is now rejected if it contains path separators or .., so a crafted --instance can't escape data_path. Same bug class as the tar zip-slip this PR already closes. Added a regression test. (commit guarding instance names)

Deferred (pre-existing, out of this PR's scope) → filed as #74:

generate_activities_pages / generate_activity_page panic on malformed data via .unwrap() in the rayon loop and the actor lookup. These unwraps predate this PR (it only threaded &AppConfig through the signatures); converting panics into graceful error propagation is a behavior change beyond the coverage/config-injection scope. Tracked in site_generator page generation panics on malformed data instead of surfacing errors #74.

templates_source built its match/strip prefix with PathBuf (\-delimited on Windows) while rust-embed uses /-delimited paths, so the embedded-template fallback loaded nothing and rendering failed with 'Template index.html not found'. Operate on /-delimited strings; add a platform-independent regression test. Surfaced by the post-merge Windows matrix after #73.

lmorchard and others added 7 commits July 24, 2026 16:24

lmorchard requested a review from Copilot July 24, 2026 23:55

Copilot started reviewing on behalf of lmorchard July 24, 2026 23:55 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

lmorchard mentioned this pull request Jul 25, 2026

site_generator page generation panics on malformed data instead of surfacing errors #74

Closed

lmorchard merged commit 9f9ef51 into main Jul 25, 2026
3 checks passed

lmorchard mentioned this pull request Jul 25, 2026

Fix embedded template loading on Windows (templates_source path separators) #75

Merged

lmorchard mentioned this pull request Jul 25, 2026

Harden site_generator page generation against malformed data (#74) #76

Merged

Navigation Menu

Expand test coverage for site_generator, importer, downloader, instance (#60) - #73

Expand test coverage for site_generator, importer, downloader, instance (#60)#73
lmorchard merged 8 commits into
mainfrom
worktree-expand-test-coverage

lmorchard commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

lmorchard commented Jul 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lmorchard commented Jul 24, 2026

What changed

Design decisions

Verification

Deferred (not in this PR)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

lmorchard commented Jul 25, 2026

Copilot review triage

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants