Remove write side-effect from index() — keep GET / side-effect free

[Copilot]

GET / was mutating _progress_store as a side-effect of page rendering: the index() route would detect stale or missing progress data and write a rebuilt snapshot back into the shared in-memory store. This coupled UI rendering to runtime state repair and made debugging harder (visiting / could silently change application state).

Changes

• novelforge/session/persistence.py

  • Added rebuild_stale_progress(completed_chapters, total_chapters, progress_token) -> dict — a named, idempotent helper that owns all progress-store write logic for crash recovery.
  • Updated restore_session_from_state() to call rebuild_stale_progress() when it encounters stale (status="running", no _live flag) or absent progress data. Recovery now happens once, at session-restore time.

• novelforge/__init__.py

  • Removed the _progress_store[token] = rebuilt write from index(). Local session_data["progress_data"] reconstruction for template rendering is preserved — it only affects the response dict, not shared state.

# Before — index() wrote back into the shared store:
session_data["progress_data"] = rebuilt
if token:
    with _progress_lock:
        _progress_store[token] = rebuilt   # ← side-effect on GET /

# After — index() is read-only; recovery lives in restore_session_from_state():
session_data["progress_data"] = rebuilt
# Progress-store recovery is handled in restore_session_from_state(),
# not here, so that GET / remains free of write side-effects.

• tests/test_app.py — Two new tests assert that GET / does not mutate _progress_store, covering both the stale-token and no-token paths.
• tests/test_session.py — TestRebuildStaleProgress (3 unit tests for the new helper) and TestRestoreSessionRebuildsBrokenProgress (4 scenario tests: stale running, missing, valid done, live running).

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• api.openai.com
  • Triggering command: /usr/bin/python python -m pytest tests/test_app.py tests/test_session.py -q --tb=short (dns block)
  • Triggering command: /usr/bin/python python -m pytest -q --tb=short (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[Copilot]

Pull request overview

This PR removes a write side-effect from GET / by relocating progress-store crash-recovery writes into the session restore lifecycle (restore_session_from_state()), keeping the index page rendering path read-only with respect to shared in-memory state.

Changes:

• Added rebuild_stale_progress() to centralize stale/missing progress reconstruction + persistence into _progress_store.
• Updated restore_session_from_state() to rebuild stale/absent progress once during restore, instead of during GET /.
• Added route/unit tests to assert GET / does not mutate _progress_store and to cover the new rebuild/restore behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
novelforge/session/persistence.py	Introduces rebuild_stale_progress() and moves stale-progress recovery into restore_session_from_state().
novelforge/__init__.py	Removes the _progress_store write from index() while preserving local reconstruction for rendering.
tests/test_app.py	Adds regression tests ensuring GET / does not mutate or insert into _progress_store.
tests/test_session.py	Adds unit + scenario tests for rebuild helper and restore-time recovery behavior.

---

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

[Copilot]

pd = state.get("progress_data") is not schema-validated, but the new logic calls pd.get(...). If a corrupted/old session file has a non-dict progress_data (e.g., a list or string), this will raise AttributeError during restore (previous code would just store it). Consider guarding with isinstance(pd, dict) (otherwise treat as missing/invalid and rebuild from completed_chapters or skip).

Suggested change

	completed = state.get("completed_chapters", [])
	if token:
	if completed and (
	not pd
	or (pd.get("status") == "running" and not pd.get("_live"))
	):
	rebuild_stale_progress(completed, state.get("chapters", 0), token)
	elif pd:
	pd_is_valid = isinstance(pd, dict)
	completed = state.get("completed_chapters", [])
	if token:
	if completed and (
	not pd_is_valid
	or not pd
	or (pd.get("status") == "running" and not pd.get("_live"))
	):
	rebuild_stale_progress(completed, state.get("chapters", 0), token)
	elif pd_is_valid and pd:

[Copilot]

restore_session_from_state() now owns progress-store restore/rebuild, but novelforge/routes/sessions.py still re-writes _progress_store[token] = state["progress_data"] after calling this function (routes/sessions.py:64-68). That post-write will overwrite any rebuilt snapshot and can reintroduce stale status="running" progress. Recommendation: remove/adjust the caller-side write so this function remains the single source of truth for progress-store restoration.

Suggested change

	elif pd:
	with _progress_lock:
	_progress_store[token] = pd
	with _progress_lock:
	restored_progress = _progress_store.get(token)
	if restored_progress is not None:
	state["progress_data"] = dict(restored_progress)
	else:
	state.pop("progress_data", None)
	elif pd:
	restored_progress = dict(pd)
	with _progress_lock:
	_progress_store[token] = restored_progress
	state["progress_data"] = dict(restored_progress)
	else:
	state.pop("progress_data", None)
	else:
	state.pop("progress_data", None)

[Copilot]

rebuild_stale_progress() stores completed_chapters into _progress_store by reference ("chapters_done": completed_chapters). Elsewhere, progress snapshots usually copy mutable lists before storing (e.g., list(chapters_done) in generation). Consider copying completed_chapters (and potentially the dict itself via dict(...)) to avoid later in-place mutations of the original list unexpectedly mutating the shared progress store.

Suggested change

	"chapters_done": completed_chapters,
	"error": None,
	}
	with _progress_lock:
	_progress_store[progress_token] = rebuilt
	"chapters_done": list(completed_chapters),
	"error": None,
	}
	with _progress_lock:
	_progress_store[progress_token] = dict(rebuilt)