Fix chapter rhythm classifier JSONDecodeError and test isolation stale-patch bug

[Copilot]

Two bugs caused test_set_step_updates_in_memory_always, test_fewer_disk_writes_than_step_updates, and test_done_only_set_after_all_post_manuscript_passes to fail when the full test suite ran together—but pass in isolation.

Root causes

1. Stale instance attribute on progress_manager singleton (primary)

test_warning_logged_when_novel_progress_entry_missing patched pm.update at the instance level. On teardown, monkeypatch restored the original bound method as an instance attribute, which Python resolves before the class. Any subsequent test that spied via monkeypatch.setattr(type(progress_manager), "update", spy) had its spy silently bypassed—step_labels and done_set_while_map_present stayed empty.

Fix: patch at the class level so teardown restores the class attribute cleanly:

# Before
original_update = pm.update
def _raise_on_novel_token(tok, data): ...
monkeypatch.setattr(pm, "update", _raise_on_novel_token)

# After
original_class_update = type(pm).update
def _raise_on_novel_token(self, tok, data): ...
monkeypatch.setattr(type(pm), "update", _raise_on_novel_token)

2. Rhythm classifier mock returning non-JSON (secondary)

_canned_llm_response had no branch for "Classifying chapter rhythm for Chapter N", so the mock returned "Processed output from the LLM agent.", triggering JSONDecodeError on every mocked pipeline run. The exception was already caught gracefully, but the noise was unnecessary and risked masking real issues.

Fix: added a handler in conftest.py:

if "classifying" in act or ("rhythm" in act and "chapter" in act):
    return json.dumps({
        "detected_patterns": [],
        "recommended_shape_for_this_chapter": "lyrical-interlude",
        "recommendation_reason": "Default rhythm for test purposes.",
    })

Additional hardening

• pipeline.py: run_chapter_rhythm_classifier fallback dict now includes explicit safe defaults "recommended_shape_for_this_chapter": "" and "recommendation_reason": "" so callers get typed values rather than relying on .get(..., "") fallbacks.
• test_chapter_agent_failures.py: updated test_rhythm_classifier_fallback_is_otherwise_empty to assert the fallback contains exactly {PASS_FAILURE_KEY, "recommended_shape_for_this_chapter", "recommendation_reason"}.

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/ -x (dns block)
  • Triggering command: /usr/bin/python python -m pytest tests/ --no-header -q (dns block)
  • Triggering command: /usr/bin/python python -m pytest tests/test_app.py tests/test_approve_outline_transactional.py tests/test_base_agent.py tests/test_blueprints.py tests/test_boundary.py tests/test_chapter_agent_failures.py tests/test_chapter_position.py tests/test_chapter_prompts.py tests/test_concurrency.py tests/test_config_shim.py tests/test_coverage.py tests/test_dotenv_bootstrap.py tests/test_ensure_app_dirs.py tests/test_export_escaping.py tests/test_export_snapshot.py tests/test_illustration_job.py tests/test_image_download_timeout.py (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 fixes two test-suite-ordering issues in NovelForge’s chapter pipeline and progress tracking tests, ensuring tests that rely on progress_manager.update spies and rhythm-classifier JSON parsing behave consistently when the full suite runs together.

Changes:

• Fixes test isolation by patching progress_manager.update at the class level (avoids stale instance attribute shadowing).
• Extends the canned LLM test responder to return valid JSON for the chapter rhythm classifier action.
• Hardens run_chapter_rhythm_classifier’s failure fallback to include explicit safe-default business keys, and updates the corresponding test assertions.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
tests/test_illustration_job.py	Switches monkeypatching of progress_manager.update from instance-level to class-level to prevent cross-test contamination.
tests/test_chapter_agent_failures.py	Updates fallback expectations for rhythm classifier to include safe-default keys and asserts no extra keys are present.
tests/conftest.py	Adds a canned JSON response branch for the chapter rhythm classifier action to avoid JSONDecodeError noise during mocked runs.
novelforge/agents/chapter/pipeline.py	Returns a richer typed fallback dict for rhythm classifier failures (includes explicit empty-string defaults).

---

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

[Copilot]

Comment says the defaults are present so callers can .get() without KeyError, but dict.get() never raises KeyError even when a key is missing. Consider rewording to reflect the real motivation (e.g., allowing direct indexing / ensuring typed keys exist for downstream code).

Suggested change

	# Minimal safe defaults must be present so callers can .get() without KeyError
	# Minimal safe defaults must be present so downstream code can rely on the expected keys.