MCQ Module Cleanup — PRD
Context
The MCQ module already ships — the mcqs collection with full admin CRUD (av1), a mobile read/sync feed (v1), and web read/list (v2). Content writes are admin-only; v1/v2 are read-only consumers. The module is healthy: an audit (asym-context/.../mcq-module-audit.txt, 2026-06-22, re-verified against HEAD bedae188 on 2026-06-25) confirmed that 20 of 21 public MCQService methods and ~30 McqRepository methods are live with production route / cross-service / bg-task callers.
This PRD is a cleanup / slim-down delta, not a feature. It removes the small set of genuinely-dead symbols the audit surfaced, plus one fully-orphaned legacy bookmark service that is the only thing keeping a dead MCQ method alive. It does three things:
- Remove dead
MCQService/McqRepositorycode — one zero-caller service method and three zero-caller repo methods. - Remove the orphaned legacy
v0_bookmark_servicesmodule — never imported, registered, or routed; replaced long ago by the liveBookmarkService/bookmark_collectionssurface. - Retire the write-orphaned
like_count/dislike_countfields — denormalized counters whose only writer is dead, never read, never serialized to any client, and (verified) carrying no meaningful data in production.
It does not re-spec the MCQ feature, the solution/encryption model, the block/docket linkage mechanisms, the sort_order / reorder mechanism, or any of the MCQ-named lookalike modules — mcq_attrs (MCQAttributesService), mcq_docket_link (McqDocketLinkService), bookmark_mcq, user_mcq_daily_stats, and the content_rating MCQ router — all of which are separate entities with their own models/services.
Scope guardrail (decided). Every change here is internal: no client-facing request or response contract changes.
like_count/dislike_countare removable within this guardrail precisely because they are not present in any response (see Data Model). Removing any field that is serialized would be out of scope.
Scope
MVP — a single-milestone, dead-code-only removal, delivered as one PR. Grouped below by layer for clarity, but there is no inter-group dependency and no sequencing within the milestone:
- Service-layer dead code. Remove
MCQService.get_by_id(sole caller is the dead v0 module) and the entire orphanedOldBookmarkService/v0_bookmark_services.pymodule; trim one stale doc-comment. - Repository-layer dead code. Remove three zero-caller
McqRepositorymethods:count_by_years,bulk_update_solution_client,update_like_dislike_count. - Dead aggregate fields. Remove
like_count/dislike_countfromMcqDBModelandMcqProjection.
M1 — MCQ dead-code removal (single PR)
Repo
get_by_idis LIVE — do not remove (decided). Only the service methodMCQService.get_by_idis dead. The repository methodmcq_repository.get_by_idis heavily used (6MCQServiceself-calls + flowchart / user-report services + exam / taxonomy bg-tasks) and is test-covered (src/tests/image_bank/test_image_bank_backfill.py:109). The two share a name; only the service one goes. This is the single most likely implementation mistake — call it out in the PR.
MCQService.get_by_idis dead via a dead caller (verified). Its only reference anywhere isservices.mcq_service.get_by_id(...)atv0_bookmark_services.py:101, insideOldBookmarkService.create_mcq_bookmark. That method, that class, and that whole module are themselves dead (below), soMCQService.get_by_idis transitively dead. No test calls it.
The whole
v0_bookmark_servicesmodule is dead (verified).v0_bookmark_services.py(OldBookmarkService, 14 methods, ~37 KB) is never imported insrc/(grep clean), not registered inunified_service_container.py, and no v0 bookmark router is mounted. Its 14 methods only reference each other. The single external trace is a stale doc-comment on the liveBookmarkTimestampCursorclass (api/v1/bookmark_collections/schemas.py:194: “…used by BookmarkService and v0_bookmark_services”). The live replacement isBookmarkService+ thebookmark_collectionssurface.
BookmarkTimestampCursorstays (decided). It is shared with the liveBookmarkService, so removing the v0 module must not remove the cursor — only trim the trailing “and v0_bookmark_services” from its docstring. Nothing else the v0 module imports becomes orphaned (all its imports — bookmark repos/projections, the cursor, the service container — are shared with live code).
Migrations kept as immutable history (decided). Two MCQ migrations are stale/broken but are retained as-is (mirrors the docket-cleanup convention):
jul_24_25/migrate_solution_admin_to_client_v2.py— callsmcq_repo.bulk_update_solution_client_v2(a differently-named method that has never existed) and targets the retiredsolution_client_v2field; it wouldAttributeErrorif ever run and is registered with no runner. Because it references a non-existent*_v2name, removingbulk_update_solution_clientdoes not affect it. Left in place.feb_20_26/migrate_mcq_solution_to_client.py— Phase-1$rename/$unsetof obsolete versioned fields is stale; Phase-2 is still valid. Untouched.
like_count/dislike_countare dead, internal, and empty (decided). They are denormalized aggregate counters whose only writer is the deadupdate_like_dislike_count; they have zero readers anywhere; they are never copied into any response (response schemas are cherry-pickfrom_projectionfactories that omit them, andMcqProjectionis never returned by a route directly); and the owner has verified production carries no meaningful values in them. They are the half-built twin ofbookmark_count, which did get wired (increment_bookmark_count←BookmarkService← bookmark bg-task, all live and retained). Removal is therefore a storage + internal-DTO change, not a response-contract change.
Removal manifest
| Group | Symbol / field to remove | Location | Why dead |
|---|---|---|---|
| Service | MCQService.get_by_id | src/services/mcq_services.py:1386 | Sole caller is the dead v0_bookmark_services; no test. |
| Service module | entire OldBookmarkService (14 methods) | src/services/v0_bookmark_services.py (whole file) | Never imported / registered / routed; only self-references + 1 stale doc-comment. |
| Doc-comment | trim “and v0_bookmark_services” from BookmarkTimestampCursor docstring | src/api/v1/bookmark_collections/schemas.py:194 | The class stays (shared with live BookmarkService); only the dangling mention goes. |
| Repo | count_by_years | src/repository/mcq_repository.py:2099 | Zero refs anywhere; the live year-count path is aggregate_mcq_counts_by_year_for_course. |
| Repo | bulk_update_solution_client | src/repository/mcq_repository.py:2678 | Zero live refs; the only mention is the broken jul_24_25 migration calling a non-existent *_v2 name. |
| Repo | update_like_dislike_count | src/repository/mcq_repository.py:2746 | Zero refs; sole writer of like_count/dislike_count. |
| Field | like_count, dislike_count (DB model) | src/models/mcq_models/db_model.py:248-249 (+ schema example :639-640) | Write-orphaned, read-inert, not serialized, no prod data. |
| Field | like_count, dislike_count (projection) | src/models/mcq_models/projection_model.py:186-187 (+ schema example :312-313) | Same; McqProjection is internal-only. |
- Test impact: 0 — no test references any removed symbol or field. (The
get_by_idtest hits in the suite targetmcq_repository.get_by_id/exam_repository.get_by_id— different, live methods.) - Dead schemas: 0 — the v0 module defines only
OldBookmarkService; every schema/projection/repo it imports is shared with live code. No response schema becomes orphaned. - Response-contract change: none —
like_count/dislike_countare absent from all av1/v1/v2 response schemas;McqProjectionis never returned to a client.
Data Model (requirements-level)
McqDBModel loses two fields; everything else is unchanged.
| Field | Change | Notes |
|---|---|---|
like_count | Removed | Denormalized “likes” counter. Only writer (update_like_dislike_count) is dead; zero readers; never serialized; no prod data. |
dislike_count | Removed | As above. |
bookmark_count | unchanged | The genuinely-wired sibling counter — kept (increment_bookmark_count ← BookmarkService ← bookmark bg-task, all live). |
| (all other MCQ fields) | unchanged | Options, solution, taxonomy/exam links, block linkage, status, version, etc. are untouched. |
Existing Mongo data left in place (decided). Stored docs may physically carry
like_count: 0/dislike_count: 0(Beanie wrote thedefault=0on insert). Dropping the Pydantic fields makes Mongo ignore those values on read (schemaless). No hard delete now; an optional$unsetcleanup migration may follow later (see Open Questions).
Delivery Milestones
| # | Milestone | Outcome | Status | Plan |
|---|---|---|---|---|
| 1 | MCQ dead-code removal | One PR: MCQService.get_by_id + the whole v0_bookmark_services module gone; 3 dead McqRepository methods gone; like_count/dislike_count retired from model + projection. Scope expanded during spec review to also drop the 4 v0-only bookmark repositories + 4 v0 bookmark model packages (transitive orphans of the v0 deletion). No behaviour, request, or response change. | ✅ complete | spec · plan · PR #793 |
Plan key:
BE= backend plan tasks in keystone (keystone/docs/superpowers/specs/+…/plans/), authored from this PRD via/plan.
Sequencing. Single milestone, one PR. The three groups (service, repo, fields) are independent and need no internal ordering — but ship and review them as one diff since the total footprint is small (~1 method + 1 module + 3 methods + 2 fields). Migrations are not touched.
Open Questions
- Optional
$unsetcleanup forlike_count/dislike_count. Field drop leaves stored0values behind (harmless; ignored on read). Do we want a follow-up$unsetmigration to physically remove them, or leave them indefinitely? (Mirrors docket’s deferred-$unsetdecision; not required for this PR.) (Still open post-merge.) - Optional drop of the 4 orphaned v0 bookmark collections (
bookmark_mcq,bookmark_docket,bookmark_mcq_bucket,bookmark_docket_bucket). Code + Beanie registration removed in PR #793; the Mongo collections remain. Drop later (manual or migration) once the removal has soaked, or leave indefinitely. (New, opened by the scope expansion; not required for this PR.) - Everything else is resolved during PRD review: dead-code only (no hygiene pass); migrations kept as immutable history; whole v0 module removed; repo
get_by_idretained;BookmarkTimestampCursorretained;like_count/dislike_countremoved as internal fields (verified no prod data, never serialized);bookmark_countretained.
Risks
| Risk | Likelihood | Impact | Mitigation |
|---|---|---|---|
Removing the wrong get_by_id. Confusing the dead MCQService.get_by_id with the live, test-covered mcq_repository.get_by_id. | Med | High — breaks update/delete/get flows + cross-service callers | Explicitly scoped: only the service method is removed. The repo method stays; the test suite (incl. test_image_bank_backfill.py) exercises it. Call out the distinction in the PR. |
Hidden caller via dynamic dispatch. A removed symbol is reached through a path static grep missed (e.g. another services.mcq_service.* alias). | Low | High — runtime AttributeError | Alias sweep already run (only services.mcq_service.get_by_id reached indirectly — its caller is the dead v0 module, also removed). Re-grep at implementation time; rely on the full test suite + a smoke pass. |
v0 module removal orphaning a shared symbol. Removing v0_bookmark_services also drops something live code still needs (e.g. BookmarkTimestampCursor). | Low | Med — import error in BookmarkService | Verified: the cursor and all other v0 imports are shared with the live BookmarkService; only the orphaned class + its self-references are removed, plus a docstring trim. |
like_count/dislike_count turn out to be serialized somewhere. A client silently depends on the field. | Low | Med — response key disappears | Verified absent from every av1/v1/v2 response schema; McqProjection is never returned to a client directly. Re-confirm at implementation; no client coordination expected. |
Stored field values cause confusion later. Left-behind like_count: 0 in Mongo. | Low | Low — cosmetic | Documented as schemaless leftovers; optional $unset migration tracked as an Open Question. |
Technical Details (as built)
Status: ✅ built & merged (2026-06-25).
- PR: #793 — refactor(mcq): remove verified dead code (MCQ + legacy v0 bookmark surface) — merged into
devon 2026-06-25. - Branch:
chore/mcq-deadcode-cleanup(7 commits: 1 docs + 6 removal commits, one per spec group). - Spec:
keystone/docs/superpowers/specs/2026-06-25-mcq-cleanup-deadcode-design.md - Plan:
keystone/docs/superpowers/plans/2026-06-25-mcq-cleanup-deadcode.md(7 tasks; removal-style gate = grep-zero +import src.mainsmoke + ruff F401, since test impact was verified 0). - Built by: Burhan Kapdawala, via the PRD → spec → plan flow. Every removal was confirmed by an 8-way adversarial caller sweep + completeness critic before implementation.
- Footprint: 17 files deleted (v0 service + 4 repos + 4 model packages = 12 files), 6 edited; ~5,270 lines removed. Implementation matched the spec exactly — no in-flight deviations from the approved spec.
Naming: PRD vs code
| PRD term | Code |
|---|---|
| MCQ entity | McqDBModel (collection mcqs) / McqProjection |
| MCQ service / repo | MCQService (src/services/mcq_services.py), mcq_repository / mcq_repo alias (src/repository/mcq_repository.py) |
| Dead service method | MCQService.get_by_id (mcq_services.py:1386) |
| Live repo method (KEEP — do not confuse) | mcq_repository.get_by_id (mcq_repository.py) |
| Orphaned legacy bookmark module | OldBookmarkService (src/services/v0_bookmark_services.py) |
| Live bookmark replacement (KEEP) | BookmarkService (src/services/bookmark_service.py) + bookmark_collections surface |
| Shared cursor (KEEP) | BookmarkTimestampCursor (src/api/v1/bookmark_collections/schemas.py) |
| Dead repo methods | count_by_years, bulk_update_solution_client, update_like_dislike_count (mcq_repository.py:2099 / 2678 / 2746) |
| Live year-count path (KEEP) | aggregate_mcq_counts_by_year_for_course (mcq_repository.py) |
| Dead aggregate fields | like_count, dislike_count (db_model.py:248-249, projection_model.py:186-187) |
| Live counter sibling (KEEP) | bookmark_count via increment_bookmark_count |
| Dead v0 bookmark repos (added to scope) | bookmark_mcq_repository.py, bookmark_docket_repository.py, bookmark_mcq_bucket_repository.py, bookmark_docket_bucket_repository.py (src/repository/) |
| Dead v0 bookmark model packages (added to scope) | bookmark_mcq_models/, bookmark_docket_models/, bookmark_mcq_bucket_models/, bookmark_docket_bucket_models/ (src/models/) — *DBModel + *Projection + create_*_unique_key |
| Beanie registration trimmed | 4 model entries removed from MODELS_TO_MIGRATE (src/db/mongodb/migrations.py) |
| Stale cursor type-hint fixed | BookmarkTimestampCursor.create_next_cursor Union[…4 old projections…] → List[BookmarkProjection] (bookmark_collections/schemas.py) |
| Live bookmark surfaces (KEEP) | BookmarkDBModel / BookmarkProjection, BookmarkCollectionDBModel / BookmarkCollectionProjection, bookmark_repository, bookmark_collection_repository — distinct from the 4 removed v0 packages |
| Kept-as-history migrations | jul_24_25/migrate_solution_admin_to_client_v2.py, feb_20_26/migrate_mcq_solution_to_client.py |
Decisions & deviations from the PRD (as built)
- Scope expanded: 4 v0-only bookmark repositories removed (not in the original PRD manifest). Reason: pre-implementation verification found that deleting
v0_bookmark_servicesleftbookmark_mcq_repository,bookmark_docket_repository,bookmark_mcq_bucket_repository, andbookmark_docket_bucket_repositorywith zero callers — they were transitive orphans of the v0 deletion (same dead-via-dead-caller logic the PRD applied toMCQService.get_by_id). The liveBookmarkServiceusesbookmark_repository/bookmark_collection_repositoryinstead. Approved during spec review. - Scope expanded: 4 v0 bookmark model packages removed (
bookmark_mcq_models/,bookmark_docket_models/,bookmark_mcq_bucket_models/,bookmark_docket_bucket_models/— eachdb_model.py+projection_model.py+__init__.py, incl. thecreate_*_unique_keyhelpers). Reason: once the 4 repos went, the only remaining references to these models were (a) the Beanie registration list and (b) one stale type hint. The 4 projections were dead-via-stale-hint —BookmarkTimestampCursor.create_next_cursordeclared aUnionof them, but the live caller passesList[BookmarkProjection](the projections weren’t even in the union). Approved during spec review. - Two consequential edits the expansion required: dropped 4 entries from
MODELS_TO_MIGRATEinsrc/db/mongodb/migrations.py(Beanie de-registration only — collections untouched), and rewired the staleUnion[…]hint toList[BookmarkProjection]inbookmark_collections/schemas.py(also dropped the now-unusedUnionimport). - No deviation on the core PRD manifest. The service-vs-repo
get_by_iddistinction held (livemcq_repository.get_by_idretained);BookmarkTimestampCursorclass retained (docstring trimmed only);bookmark_count/increment_bookmark_countretained; both stale MCQ migrations left as immutable history. $unsetforlike_count/dislike_countdeferred (decided) — see Open Questions / Migration.
Migration
- Code-only PR — no data migration, no collection drop. Existing
like_count/dislike_countvalues in storedmcqsdocs are left in place (schemaless, ignored on read; MCQ models have noextra="forbid"). Optional$unsetcleanup migration may follow once the field removal has soaked (see Open Questions). - 4 v0 bookmark collections left in Mongo. De-registering the 4 models from
MODELS_TO_MIGRATEonly stops Beanie from managing them — thebookmark_mcq,bookmark_docket,bookmark_mcq_bucket, andbookmark_docket_bucketcollections persist untouched (decided: no migration to clear DB now; optional future drop tracked alongside the$unsetfollow-up). - The two stale MCQ migrations (
jul_24_25,feb_20_26) are retained as immutable history and are not run or modified.