fix(manifest): do not panic on corrupt manual_sources.json (#48) #53

Merged

rawdaGastan merged 2 commits from development_fix_manifest_panic into development

2026-04-26 08:14:24 +00:00

rawdaGastan commented

2026-04-22 16:37:08 +00:00

Member

Summary

Corrupt or malformed manual_sources.json no longer crashes hero_router. The startup path, hero_router list, and hero_router scan all now log a structured WARN and proceed with an empty in-memory manifest; the next write overwrites the bad file.

The mutating CLI commands (hero_router add / remove) keep their ?-propagating behavior — they still fail loudly on a bad file, as a user running them should see the parse error rather than silently lose their sources.

Closes #48.

Changes

`crates/hero_router/src/manifest.rs`

New Manifest::empty(path: PathBuf) -> Self constructor — in-memory empty manifest bound to path, no disk I/O.
#[derive(Debug)] on Manifest (required by the new test's unwrap_err()).
Two unit tests:
- empty_then_save_writes_valid_file_at_given_path — roundtrip through add + save + load.
- load_fails_on_malformed_json — documents the failure mode.

`crates/hero_router/src/main.rs`

New private helper load_manifest_or_empty(path: &Path) -> Manifest at module scope. Logs via tracing::warn! with structured path and error fields, then returns Manifest::empty(path.to_path_buf()).
Routed all three read-only call sites through the helper, replacing three near-identical unwrap_or_else(|_| panic!(...)) blocks:
- build_and_start (was a /dev/null fallback that itself panicked)
- cmd_list (was a load_default() fallback pointing at the same file)
- cmd_scan (same pattern as cmd_list)

`crates/hero_router/src/server/rpc.rs`, `crates/hero_router/src/server/terminal.rs`

Pure rustfmt reflows auto-applied during the test run. No semantic changes.

Why not `/dev/null` as the fallback?

The original code's intended safe fallback doesn't work:

/dev/null exists on Linux, so Manifest::load passes path.exists(), reads "", and serde_json::from_str("") fails — the fallback itself panics, so the outer unwrap_or_else does too.
Even if Manifest::load were taught to treat empty as Vec::new(), the resulting Manifest.path would be /dev/null, so every subsequent save() would silently discard user data (adds from the UI would appear to succeed but persist nothing).
/dev/null doesn't exist on Windows.
Manifest::empty(path) keeps the correct persistence target, so the next write self-heals the bad file.

Test Results

cargo test --workspace — 73 passed / 0 failed / 0 ignored (2 new tests in manifest::tests).
cargo clippy --workspace --all-targets -- -D warnings — clean.
cargo fmt --all --check — clean.

End-to-end verification against the issue repro

$ echo 'not-json' > ~/.hero/router/manual_sources.json

$ hero_router list
WARN hero_router: failed to load manual_sources manifest; starting with empty manifest
  path=/home/.../manual_sources.json
  error=Failed to parse manifest: /home/.../manual_sources.json
(exit 0, table rendered normally)

$ hero_router remove some-id
Error: Failed to parse manifest: /home/.../manual_sources.json
  Caused by: expected ident at line 1 column 2
(exit 1, fails loudly as designed)

## Summary Corrupt or malformed `manual_sources.json` no longer crashes `hero_router`. The startup path, `hero_router list`, and `hero_router scan` all now log a structured `WARN` and proceed with an empty in-memory manifest; the next write overwrites the bad file. The mutating CLI commands (`hero_router add` / `remove`) keep their `?`-propagating behavior — they still fail loudly on a bad file, as a user running them should see the parse error rather than silently lose their sources. Closes https://forge.ourworld.tf/lhumina_code/hero_router/issues/48. ## Changes ### `crates/hero_router/src/manifest.rs` - New `Manifest::empty(path: PathBuf) -> Self` constructor — in-memory empty manifest bound to `path`, no disk I/O. - `#[derive(Debug)]` on `Manifest` (required by the new test's `unwrap_err()`). - Two unit tests: - `empty_then_save_writes_valid_file_at_given_path` — roundtrip through `add` + `save` + `load`. - `load_fails_on_malformed_json` — documents the failure mode. ### `crates/hero_router/src/main.rs` - New private helper `load_manifest_or_empty(path: &Path) -> Manifest` at module scope. Logs via `tracing::warn!` with structured `path` and `error` fields, then returns `Manifest::empty(path.to_path_buf())`. - Routed all three read-only call sites through the helper, replacing three near-identical `unwrap_or_else(|_| panic!(...))` blocks: - `build_and_start` (was a `/dev/null` fallback that itself panicked) - `cmd_list` (was a `load_default()` fallback pointing at the same file) - `cmd_scan` (same pattern as `cmd_list`) ### `crates/hero_router/src/server/rpc.rs`, `crates/hero_router/src/server/terminal.rs` - Pure rustfmt reflows auto-applied during the test run. No semantic changes. ## Why not `/dev/null` as the fallback? The original code's intended safe fallback doesn't work: 1. `/dev/null` exists on Linux, so `Manifest::load` passes `path.exists()`, reads `""`, and `serde_json::from_str("")` fails — the fallback itself panics, so the outer `unwrap_or_else` does too. 2. Even if `Manifest::load` were taught to treat empty as `Vec::new()`, the resulting `Manifest.path` would be `/dev/null`, so every subsequent `save()` would silently discard user data (adds from the UI would appear to succeed but persist nothing). 3. `/dev/null` doesn't exist on Windows. 4. `Manifest::empty(path)` keeps the correct persistence target, so the next write self-heals the bad file. ## Test Results - `cargo test --workspace` — 73 passed / 0 failed / 0 ignored (2 new tests in `manifest::tests`). - `cargo clippy --workspace --all-targets -- -D warnings` — clean. - `cargo fmt --all --check` — clean. ## End-to-end verification against the issue repro ``` $ echo 'not-json' > ~/.hero/router/manual_sources.json $ hero_router list WARN hero_router: failed to load manual_sources manifest; starting with empty manifest path=/home/.../manual_sources.json error=Failed to parse manifest: /home/.../manual_sources.json (exit 0, table rendered normally) $ hero_router remove some-id Error: Failed to parse manifest: /home/.../manual_sources.json Caused by: expected ident at line 1 column 2 (exit 1, fails loudly as designed) ```

rawdaGastan added 1 commit

2026-04-22 16:37:08 +00:00

fix(manifest): do not panic on corrupt manual_sources.json

Build & Test / check (pull_request) Successful in 1m36s

Details

Build & Test / check (push) Successful in 2m26s

Details

5ec93bed2a

#48

- Add Manifest::empty(path) constructor that builds an in-memory empty
  manifest bound to the given path, without touching disk.
- Add a private load_manifest_or_empty helper in main.rs that logs the
  parse error via tracing::warn! with structured path/error fields and
  falls back to Manifest::empty.
- Route the three read-only call sites (build_and_start, cmd_list,
  cmd_scan) through the helper, replacing the broken fallbacks that
  panicked on any parse error (the /dev/null and load_default variants
  both fed back into the same failing file).
- Keep cmd_add / cmd_remove propagating errors via ? — mutating commands
  must not silently overwrite a user's file.
- Add unit tests for Manifest::empty roundtrip and for the malformed-JSON
  failure mode.

Verified end-to-end by running the exact repro from the issue body:
hero_router list against a corrupt manifest now logs a warning and
exits 0 instead of panicking.