Findings from the feature-wide code review of the hollow topic appearance pruning feature (deployed via commits 1e38996..ceaedb9). The critical issue (missing meeting_id on GenerateTopicBriefingJob dispatch) was hotfixed in ceaedb9. These are the remaining items.
Design spec: docs/superpowers/specs/2026-04-11-prune-hollow-topic-appearances-design.md
Implementation plan: docs/superpowers/plans/2026-04-11-prune-hollow-topic-appearances.md
Important
1. prompt_templates:populate clobbers admin UI edits without warning
lib/tasks/prompt_templates.rake (the populate task) unconditionally calls template.update!(**attrs) on every template, overwriting whatever is in the DB. When running bin/rails prompt_templates:populate on production to deploy a prompt change, any edits made via the admin UI since the last populate run are silently clobbered.
This is not a new problem introduced by the pruning feature — the task has always worked this way — but the pruning feature is the first to depend on running populate on production after a code change as part of the deploy workflow. The failure mode is invisible: "why did my admin edit disappear."
Suggested fix: Add a --force flag (or a diff-and-confirm step) to the populate task. At minimum, update the task description to document the clobber behavior. Consider a --dry-run mode that shows which templates would change.
2. last_activity_at recomputation in the 2+ case lacks a "never go backward" guard
app/jobs/prune_hollow_appearances_job.rb around line 135 (the 2+ case in demote_topic):
```ruby
else
2+ remaining: recompute last_activity_at so homepage reflects the
cleaned appearance set (in case the most recent pruned appearance
was the one driving last_activity_at).
topic.update!(last_activity_at: new_last_activity)
end
```
The 0 and 1 cases intentionally overwrite last_activity_at (nil for 0, recomputed for 1). The 2+ case also overwrites unconditionally. But Motion#after_create in app/models/motion.rb also writes last_activity_at directly on the topic, and Topics::ContinuityService uses a "never go backward" guard (`if @topic.last_activity_at.nil? || @topic.last_activity_at < last`).
Low-probability race: if a motion is created on this topic on a different meeting concurrently with the prune job, the prune job could overwrite a newer last_activity_at with a stale one. Safe failure direction (topic temporarily ages out of homepage then bounces back), but worth matching the continuity service pattern.
Suggested fix: In the 2+ case only, guard the update: `topic.update!(last_activity_at: new_last_activity) if topic.last_activity_at != new_last_activity`. Or better, match the continuity service's monotonic logic: only write if the new value is `nil` or newer than the current value. The 0 and 1 cases should stay as-is (the explicit overwrite is intentional there).
3. Procedural-filter false-positive risk is real and unmitigated
app/jobs/prune_hollow_appearances_job.rb line 102 inside hollow?:
```ruby
Procedural filter: missing entry on a new-format summary means the
AI filtered this item as procedural — eligible for pruning.
return true if entry.nil?
```
The analyze_meeting_content prompt instructs the AI to exclude procedural items from item_details. But it does NOT guarantee every non-procedural item appears. Failure modes:
- AI context-length pressure: long packets/transcripts with many agenda items → AI may miss a legitimate item.
- Title normalization mismatch: AI produces a slightly different title that fails
normalize_title matching → falls through to entry.nil?.
The motion-rescue safety net catches any item that produced a formal motion, but substantive discussion items without a vote would not be rescued.
Suggested fix: Two parts.
- Add a comment at line 102 explaining the known failure mode and mitigation (motion rescue). Current comment only explains intent.
- Log a warning when this branch fires so false-positive rates can be monitored: `Rails.logger.info("PruneHollowAppearancesJob: procedural prune on #{agenda_item.title} (meeting #{agenda_item.meeting_id})")`.
If false positives become visible in practice, consider tightening the rule (e.g., require BOTH `entry.nil?` AND `AgendaItemTopic` age > N days, so freshly-extracted associations get a grace period).
Minor
4. Test coverage gap: item_details key present but value is nil
`test/jobs/prune_hollow_appearances_job_test.rb` — the job's guard `return unless item_details.is_a?(Array)` handles `nil` `item_details`, but there's no test asserting the behavior for `generation_data: { "item_details" => nil }`. This is a valid JSON shape the AI can produce. The guard is correct; the gap means future refactors could silently break it.
Suggested fix: Add a one-line test case wrapping the existing "returns early when meeting has no summary" pattern but with an explicit `item_details: nil`.
5. normalize_title does not handle Roman numeral section headers
`app/jobs/prune_hollow_appearances_job.rb` around line 86. The regex `\A\s*\d+(-\d+)?[a-z]?\.?\s*` strips Arabic numeral prefixes and YY-NNN council numbers but not Roman numeral section headers like `III. PUBLIC COMMENT` or `IV.A. ORDINANCE ITEMS`. These appear occasionally in Two Rivers agendas. If the AI normalizes them differently in `item_details`, the match fails and the item falls through to the procedural-filter branch.
Low risk — Roman-numeral items are usually high-level section headers that wouldn't have topics attached — but worth noting.
Suggested fix: Extend the regex to also strip leading Roman numerals: `\A\s*(?:[IVX]+\.?[A-Z]?\.?|\d+(-\d+)?[a-z]?\.?)\s*`. Add a test case for `"III. PUBLIC COMMENT"`.
6. TopicStatusEvent.evidence_type has no inclusion: validation
`app/models/topic_status_event.rb` — validates `presence: true` on `evidence_type` but does not restrict allowed values. The new `"hollow_appearance_prune"` string works fine, but a typo in the evidence type would silently create an invalid row.
Suggested fix: Add an inclusion validation listing the current known types: `rules_engine_update`, `deferral_signal`, `cross_body_progression`, `hollow_appearance_prune`. Pre-existing code paths may need a grep to confirm the full list.
Priority
- 3 is the most operationally important — it's the main "how could this overcorrect in production" failure mode, and we should start logging to monitor it.
- 1 is a latent footgun anyone running `prompt_templates:populate` on prod could hit.
- 2 is low-probability but a 5-minute fix.
- 4-6 are hygiene; bundle or defer.
Fix at leisure. None are blocking.
Findings from the feature-wide code review of the hollow topic appearance pruning feature (deployed via commits
1e38996..ceaedb9). The critical issue (missingmeeting_idonGenerateTopicBriefingJobdispatch) was hotfixed inceaedb9. These are the remaining items.Design spec:
docs/superpowers/specs/2026-04-11-prune-hollow-topic-appearances-design.mdImplementation plan:
docs/superpowers/plans/2026-04-11-prune-hollow-topic-appearances.mdImportant
1.
prompt_templates:populateclobbers admin UI edits without warninglib/tasks/prompt_templates.rake(thepopulatetask) unconditionally callstemplate.update!(**attrs)on every template, overwriting whatever is in the DB. When runningbin/rails prompt_templates:populateon production to deploy a prompt change, any edits made via the admin UI since the last populate run are silently clobbered.This is not a new problem introduced by the pruning feature — the task has always worked this way — but the pruning feature is the first to depend on running
populateon production after a code change as part of the deploy workflow. The failure mode is invisible: "why did my admin edit disappear."Suggested fix: Add a
--forceflag (or a diff-and-confirm step) to the populate task. At minimum, update the task description to document the clobber behavior. Consider a--dry-runmode that shows which templates would change.2.
last_activity_atrecomputation in the 2+ case lacks a "never go backward" guardapp/jobs/prune_hollow_appearances_job.rbaround line 135 (the 2+ case indemote_topic):```ruby
else
2+ remaining: recompute last_activity_at so homepage reflects the
cleaned appearance set (in case the most recent pruned appearance
was the one driving last_activity_at).
topic.update!(last_activity_at: new_last_activity)
end
```
The 0 and 1 cases intentionally overwrite
last_activity_at(nil for 0, recomputed for 1). The 2+ case also overwrites unconditionally. ButMotion#after_createinapp/models/motion.rbalso writeslast_activity_atdirectly on the topic, andTopics::ContinuityServiceuses a "never go backward" guard (`if @topic.last_activity_at.nil? || @topic.last_activity_at < last`).Low-probability race: if a motion is created on this topic on a different meeting concurrently with the prune job, the prune job could overwrite a newer
last_activity_atwith a stale one. Safe failure direction (topic temporarily ages out of homepage then bounces back), but worth matching the continuity service pattern.Suggested fix: In the 2+ case only, guard the update: `topic.update!(last_activity_at: new_last_activity) if topic.last_activity_at != new_last_activity`. Or better, match the continuity service's monotonic logic: only write if the new value is `nil` or newer than the current value. The 0 and 1 cases should stay as-is (the explicit overwrite is intentional there).
3. Procedural-filter false-positive risk is real and unmitigated
app/jobs/prune_hollow_appearances_job.rbline 102 insidehollow?:```ruby
Procedural filter: missing entry on a new-format summary means the
AI filtered this item as procedural — eligible for pruning.
return true if entry.nil?
```
The
analyze_meeting_contentprompt instructs the AI to exclude procedural items fromitem_details. But it does NOT guarantee every non-procedural item appears. Failure modes:normalize_titlematching → falls through toentry.nil?.The motion-rescue safety net catches any item that produced a formal motion, but substantive discussion items without a vote would not be rescued.
Suggested fix: Two parts.
If false positives become visible in practice, consider tightening the rule (e.g., require BOTH `entry.nil?` AND `AgendaItemTopic` age > N days, so freshly-extracted associations get a grace period).
Minor
4. Test coverage gap:
item_detailskey present but value isnil`test/jobs/prune_hollow_appearances_job_test.rb` — the job's guard `return unless item_details.is_a?(Array)` handles `nil` `item_details`, but there's no test asserting the behavior for `generation_data: { "item_details" => nil }`. This is a valid JSON shape the AI can produce. The guard is correct; the gap means future refactors could silently break it.
Suggested fix: Add a one-line test case wrapping the existing "returns early when meeting has no summary" pattern but with an explicit `item_details: nil`.
5.
normalize_titledoes not handle Roman numeral section headers`app/jobs/prune_hollow_appearances_job.rb` around line 86. The regex `\A\s*\d+(-\d+)?[a-z]?\.?\s*` strips Arabic numeral prefixes and YY-NNN council numbers but not Roman numeral section headers like `III. PUBLIC COMMENT` or `IV.A. ORDINANCE ITEMS`. These appear occasionally in Two Rivers agendas. If the AI normalizes them differently in `item_details`, the match fails and the item falls through to the procedural-filter branch.
Low risk — Roman-numeral items are usually high-level section headers that wouldn't have topics attached — but worth noting.
Suggested fix: Extend the regex to also strip leading Roman numerals: `\A\s*(?:[IVX]+\.?[A-Z]?\.?|\d+(-\d+)?[a-z]?\.?)\s*`. Add a test case for `"III. PUBLIC COMMENT"`.
6.
TopicStatusEvent.evidence_typehas noinclusion:validation`app/models/topic_status_event.rb` — validates `presence: true` on `evidence_type` but does not restrict allowed values. The new `"hollow_appearance_prune"` string works fine, but a typo in the evidence type would silently create an invalid row.
Suggested fix: Add an inclusion validation listing the current known types: `rules_engine_update`, `deferral_signal`, `cross_body_progression`, `hollow_appearance_prune`. Pre-existing code paths may need a grep to confirm the full list.
Priority
Fix at leisure. None are blocking.