Skip to content

fix(recitation): prefer playbook.db over playbook.json when both exist#20

Merged
azalio merged 4 commits intomainfrom
fix/recitation-prefer-playbook-db
Nov 2, 2025
Merged

fix(recitation): prefer playbook.db over playbook.json when both exist#20
azalio merged 4 commits intomainfrom
fix/recitation-prefer-playbook-db

Conversation

@azalio
Copy link
Copy Markdown
Owner

@azalio azalio commented Nov 2, 2025

Summary

Fixes issue where map-efficient (and other commands using RecitationManager) showed "Error reading file" for .claude/playbook.json despite the database already being migrated to .claude/playbook.db.

Problem

When both .claude/playbook.db and .claude/playbook.json existed, RecitationManager was incorrectly passing the JSON file path to PlaybookManager, causing it to attempt reading the deprecated JSON file even though the SQLite database was the current source of truth.

Solution

Updated RecitationManager initialization logic in src/mapify_cli/recitation_manager.py:

  • Only pass playbook_path parameter if .db doesn't exist yet
  • Prefer .db if it exists, only fall back to .json for migration
  • Maintain backward compatibility with JSON-only setups

Changes

  • Modified: src/mapify_cli/recitation_manager.py (line 562)
    • Changed condition from playbook_json_path.exists() to not playbook_db_path.exists() and playbook_json_path.exists()
  • Added: tests/test_recitation_playbook_db_fix.py
    • Test DB preference when both files exist
    • Test migration still works when only JSON exists
    • Test graceful handling of missing playbook

Test Results

All 3 new tests pass:

  • test_recitation_prefers_db_over_json
  • test_recitation_migrates_json_to_db_when_only_json_exists
  • test_recitation_handles_missing_playbook_gracefully

All existing playbook/recitation tests still pass (210 tests).

Verification

# Test the fix works
mapify recitation get-context  # No longer shows JSON read error

# Run new tests
pytest tests/test_recitation_playbook_db_fix.py -v

# Run all related tests
pytest tests/ -k "playbook or recitation" -v

Related

  • Migration logic in PlaybookManager.__init__() already handles JSON→SQLite migration correctly
  • This fix ensures RecitationManager doesn't interfere with that logic

When both .claude/playbook.db and .claude/playbook.json existed,
RecitationManager was incorrectly passing playbook.json path to
PlaybookManager, causing it to try reading the JSON file even though
the DB was already migrated.

Changes:
- Update RecitationManager to only pass playbook_path if DB doesn't exist yet
- Add comprehensive tests for DB preference and migration scenarios
- Ensure backward compatibility with JSON-only setups

Fixes issue where map-efficient showed "Error reading file" for playbook.json
despite successful migration to SQLite format.

Tests:
- test_recitation_prefers_db_over_json: Verifies DB is used when both exist
- test_recitation_migrates_json_to_db_when_only_json_exists: Migration still works
- test_recitation_handles_missing_playbook_gracefully: No errors when missing
Copilot AI review requested due to automatic review settings November 2, 2025 15:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue where RecitationManager was incorrectly attempting to read playbook.json even when playbook.db already existed, causing unnecessary migration attempts. The fix ensures that when both files exist, only the database file is used.

  • Modified the playbook_path parameter logic in RecitationManager.generate_context_md() to only pass the JSON path when the database doesn't exist
  • Added comprehensive test coverage for three scenarios: preferring DB over JSON, migrating JSON to DB, and handling missing playbooks

Reviewed Changes

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

File Description
src/mapify_cli/recitation_manager.py Updated PlaybookManager instantiation to prefer .db file and only pass playbook_path when migration is needed
tests/test_recitation_playbook_db_fix.py Added new test file with three test cases covering DB preference, migration, and graceful handling of missing files
Comments suppressed due to low confidence (1)

tests/test_recitation_playbook_db_fix.py:8

  • Import of 'tempfile' is not used.
import tempfile

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

Comment thread tests/test_recitation_playbook_db_fix.py Outdated
Comment thread tests/test_recitation_playbook_db_fix.py
Comment thread tests/test_recitation_playbook_db_fix.py Outdated
azalio and others added 3 commits November 2, 2025 18:27
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@azalio azalio merged commit 296bc3a into main Nov 2, 2025
6 checks passed
azalio added a commit that referenced this pull request Feb 13, 2026
…-35)

MEDIUM fixes:
- #8: Remove dead RETRY_LOOP phase from orchestrator STEP_PHASES
- #10: Fix plan path to branch-scoped .map/<branch>/task_plan_<branch>.md
- #11: Fix findings path to branch-scoped .map/<branch>/findings_<branch>.md
- #12: Remove references to non-existent ralph-loop-config.json
- #13/#14: Rewrite map-resume to use step_state.json instead of progress.md
- #15: Fix INIT_PLAN heading format (### ST-XXX with - **Status:** prefix)
- #16: Fix regex in step_runner to match plan format (### heading, - **Status:**)
- #17: Fix map-learn contradiction about automatic learning

LOW fixes:
- #9/#31: Document dual state file system (step_state.json vs workflow_state.json)
- #19: Document intentional Evaluator/Reflector/Curator omission in map-efficient
- #20: Fix line count reference (~150 → ~540 lines)
- #21: Standardize all AskUserQuestion to Python function call syntax
- #22: Rename Steps 2.5/2.6 to 2a/2b to avoid phase number collision
- #23/#24: Fix map-debate comparison table (map-efficient uses single Actor)
- #25: Replace cat commands with Read tool comments in map-check
- #28/#29: Replace undefined thrashing_detected()/max_redecompositions
- #30: Add - **Status:** pending field to map-plan template
- #32: Note map-fast max 3 vs map-efficient max 5 intentional difference
- #33: Remove Evaluator from map-fast skipped agents list
- #34: Move AskUserQuestion to "Built-in Tools" section in map-release
- #35: Replace parallel bash & processes with sequential && in map-release

Template sync: All .claude/ changes mirrored to src/mapify_cli/templates/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants