docs(minerva-rls): tighten V41 header comment — resolve dangling "see below" in (4b) rationale #4

New issue

Closed

opened 2026-04-20 18:22:44 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-04-20 18:22:44 +00:00

Collaborator

Context

Surfaced by review-orchestrator pass on mike/Minerva PR #269 (V41 cross-tenant canvasser_id binding, closes Minerva #263). Non-blocking comment-accuracy nit from the consolidated reviewer panel; filed here because it's polish, not a Minerva-functional item, and the orchestrator policy excludes .sql files from the auto-fix lane.

Problem

The V41 migration header (services/migrations/sql/V41__cross_tenant_canvasser_id_binding.sql, lines 36-39) poses a rhetorical question and answers it with an indirect reference:

(4b) canvass_contact_responses_insert — same pattern on its
     campaign_manager / field_organizer branch (V9 shape; V38 left
     this policy unchanged). Also add the explicit visit-scope check
     to the campaign_manager branch? No — see below.

The "see below" is never explicitly delivered. A careful reader can infer the answer from lines 56-60 (the "subquery shape" paragraph about canvassers_three_axis SELECT gating and client_id predicate) plus knowledge of V9's existing canvass_contact_responses.client_id = current_client_id() tenant check, but it's left to the reader to assemble.

Intended reasoning (what should be on the page)

The campaign_manager branch of canvass_contact_responses_insert does not need an explicit visit-scope EXISTS check because:

canvass_contact_responses.client_id = current_client_id() already rejects any row that names a visit_id in a different tenant (the visit's row would FK-validate, but the response's own client_id predicate rejects).
Within-tenant, the campaign_manager role is cross-scope by V9 design (no scope gate on their branch anywhere in V9 — they can enter paper reports for any of their tenant's visits regardless of scope overlap). Adding a scope check here would regress V9 semantics for legitimate paper-entry workflows.
The V41 same-tenant canvasser_id subquery handles the attribution-forgery axis that this PR is actually scoped to close.

Fix

Add a short clarifying paragraph after line 40 in a future migration header (V41 itself is checksum-locked once applied to a target DB — fix needs to land in a Vnn__... successor that touches this header comment area, e.g. as part of an unrelated canvass_contact_responses cleanup, or as a dedicated V-file if one is warranted for documentation-only migrations).

Or — lighter touch — append the reasoning as an additional COMMENT ON POLICY canvass_contact_responses_insert line in a small follow-up migration. That leaves the V41 checksum alive and documents the intent on the live policy object.

Priority

Low. Comment polish, no correctness implication. No deadline.

Cross-reference

Minerva PR #269 (V41) — mike/Minerva#269
Minerva issue #263 (V41 acceptance criteria)
V38 migration (Minerva #208) — the predecessor header this V41 header is correcting

## Context Surfaced by review-orchestrator pass on `mike/Minerva` PR #269 (V41 cross-tenant canvasser_id binding, closes Minerva #263). Non-blocking comment-accuracy nit from the consolidated reviewer panel; filed here because it's polish, not a Minerva-functional item, and the orchestrator policy excludes `.sql` files from the auto-fix lane. ## Problem The V41 migration header (`services/migrations/sql/V41__cross_tenant_canvasser_id_binding.sql`, lines 36-39) poses a rhetorical question and answers it with an indirect reference: ``` (4b) canvass_contact_responses_insert — same pattern on its campaign_manager / field_organizer branch (V9 shape; V38 left this policy unchanged). Also add the explicit visit-scope check to the campaign_manager branch? No — see below. ``` The "see below" is never explicitly delivered. A careful reader can infer the answer from lines 56-60 (the "subquery shape" paragraph about `canvassers_three_axis` SELECT gating and `client_id` predicate) plus knowledge of V9's existing `canvass_contact_responses.client_id = current_client_id()` tenant check, but it's left to the reader to assemble. ## Intended reasoning (what should be on the page) The campaign_manager branch of `canvass_contact_responses_insert` does not need an explicit visit-scope EXISTS check because: 1. `canvass_contact_responses.client_id = current_client_id()` already rejects any row that names a `visit_id` in a different tenant (the visit's row would FK-validate, but the response's own client_id predicate rejects). 2. Within-tenant, the campaign_manager role is cross-scope by V9 design (no scope gate on their branch anywhere in V9 — they can enter paper reports for any of their tenant's visits regardless of scope overlap). Adding a scope check here would regress V9 semantics for legitimate paper-entry workflows. 3. The V41 same-tenant canvasser_id subquery handles the attribution-forgery axis that this PR is actually scoped to close. ## Fix Add a short clarifying paragraph after line 40 in a future migration header (V41 itself is checksum-locked once applied to a target DB — fix needs to land in a `Vnn__...` successor that touches this header comment area, e.g. as part of an unrelated `canvass_contact_responses` cleanup, or as a dedicated V-file if one is warranted for documentation-only migrations). Or — lighter touch — append the reasoning as an additional `COMMENT ON POLICY canvass_contact_responses_insert` line in a small follow-up migration. That leaves the V41 checksum alive and documents the intent on the live policy object. ## Priority Low. Comment polish, no correctness implication. No deadline. ## Cross-reference - Minerva PR #269 (V41) — https://code.rinaldifamily.org/mike/Minerva/pulls/269 - Minerva issue #263 (V41 acceptance criteria) - V38 migration (Minerva #208) — the predecessor header this V41 header is correcting

claude-bot commented

2026-04-20 18:26:23 +00:00

Author

Collaborator