Feat: Parental Consent Flow

[ToxicBiohazard]

Types of changes

• Bugfix
• New feature
• Breaking change
• Documentation

Summary

Parental-consent / minor-report flow for Discord: /flag_minor for MOD+, review UI in a dedicated channel, /minor_reviewers (ADMIN) for who can use the buttons. Consent is checked via Nexus (POST …/discord/user_lookup/parental_consent_exists with Bearer auth and discord_id), not the old HMAC Cloud Function.

Config: NEXUS_API_BASE_URL, NEXUS_API_TOKEN, ROLE_VERIFIED_MINOR, CHANNEL_MINOR_REVIEW (plus existing bot settings).

DB: minor_report, minor_review_reviewer; associated_ban_id → ban.id (FK, ON DELETE SET NULL).

Other: ban_member_with_epoch returns ban_id on the response (incl. existing-ban path). Age-based minor-role cleanup uses dateutil.relativedelta in ScheduledTasks.

See inline review threads for the original feedback; all seven are addressed on this branch.

[codecov]

Codecov Report

❌ Patch coverage is 75.57377% with 149 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.99%. Comparing base (b9b84c3) to head (eda791a).

Files with missing lines	Patch %	Lines
src/views/minorreportview.py	54.62%	108 Missing ⚠️
src/helpers/minor_verification.py	84.07%	18 Missing ⚠️
src/cmds/automation/scheduled_tasks.py	79.31%	12 Missing ⚠️
src/cmds/core/flag_minor.py	89.52%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   65.36%   66.99%   +1.63%     
==========================================
  Files          52       58       +6     
  Lines        3069     3672     +603     
==========================================
+ Hits         2006     2460     +454     
- Misses       1063     1212     +149     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dimoschi]

Thorough implementation with good edge case handling. A few issues to address before merge.

[dimoschi]

Architectural concern: Cloud Function dependency

I have a concern about introducing a Cloud Function as a dependency for the consent verification flow.

This is a compliance-critical path (COPPA/GDPR-K), and it creates a hard coupling between the Discord bot and external cloud infrastructure:

• Availability — if the Cloud Function is down, consent verification fails silently (returns False = "no consent")
• Operational ownership — who maintains the function? Different repo/team means coordination overhead for API contract changes, secret rotation, deployments
• Infrastructure sprawl — the bot now needs network access to cloud infra, a shared HMAC secret to keep in sync, and monitoring on the function

Hackster already runs a FastAPI server alongside the bot (src/webhooks/server.py via Hypercorn). The consent form could be hosted there instead:

1. Bot generates a signed consent URL pointing to its own FastAPI server (e.g. /consent/verify?token=<signed_token>)
2. Parent opens the link → FastAPI serves a minimal HTML consent form
3. Parent submits → endpoint validates the token, records consent directly in Hackster DB (minor_report table)
4. Recheck button reads from the DB — no external call needed

This keeps the entire flow self-contained: no Cloud Function, no cross-system HMAC sync, no availability dependency on external infra. The bot already has everything it needs.

[ToxicBiohazard]

Thank you @dimoschi for your input with in-depth code check to point out several issues. Before working on fixing on all of it.

I'll revist the decision choice as per your suggestion with a different approach. I'll get back to you on Slack once I'm back from vacation.

[dimoschi]

Review Summary

Overall Assessment: Request changes 🔄

The feature is well-structured and follows existing project patterns. Good test coverage (~1775 lines of tests). However, my previous review comments have not been addressed, and the architectural concern about the Cloud Function dependency remains unresolved.

Unaddressed prior review comments

None of the 7 inline comments from my previous review have been fixed. The commits after the initial feature work (e144014, 1e89675, 8cc73bb) only address unrelated interaction/ban-decision fixes. Specifically, these are still open:

1. HMAC-SHA1 on a compliance-critical path (minor_verification.py:55) — should be SHA256 minimum for COPPA/GDPR-K data
2. Race condition in ban ID retrieval (minorreportview.py:183-188) — querying "most recent ban by user_id" can return the wrong ban under concurrency; ban_member_with_epoch should return the ID it created
3. Double status write (minorreportview.py:404-430) — status written via update_report_status() at line 404, then again manually at lines 424-430
4. get_member_or_user returning User without .roles (scheduled_tasks.py:136-142) — will raise AttributeError
5. Leap year drift in age calculation (minor_verification.py:160, scheduled_tasks.py:132) — timedelta(days=365*years) drifts ~4 days over 17 years; use dateutil.relativedelta
6. Missing ForeignKey on associated_ban_id (migration) — no referential integrity constraint

(The __tablename__ concern is actually fine, the Base class auto-generates snake_case names correctly.)

Cloud Function architecture (blocking)

I raised the architectural suggestion to remove the Cloud Function dependency in my previous review. You acknowledged it and mentioned you would revisit the approach. This has not happened yet. The concern stands:

• The bot already runs a FastAPI server (src/webhooks/server.py). The consent form and verification can be self-hosted there.
• The current design creates a hard coupling to external cloud infrastructure on a compliance-critical path. If the Cloud Function is down, consent checks silently return False ("no consent found"), which could block legitimate unbans.
• It introduces cross-system HMAC secret synchronization, separate infrastructure to maintain/monitor, and network access requirements that don't need to exist.

Self-hosting the consent endpoint in the existing FastAPI server would eliminate all of these concerns and keep the entire flow within a single deployable unit.

Additional findings

• bot.py:79 — bot.load_extension(...) should be self.load_extension(...). The module-level bot variable works at runtime but is fragile. Also, print() on lines 77-82 should use logger.
• config.py:97 — VERIFIED_MINOR: int has no default, making it required for all environments. Consider defaulting to 0 like MINOR_REVIEW in Channels.
• minorreportview.py:14 — from src.core import settings # noqa: F401 is unused; remove it.
• minor_reviewers.py:20 — Hardcoded Discord user IDs in source. Consider moving to config or removing the seed command entirely since reviewers are already managed dynamically via slash commands.

Please address the prior review comments and the Cloud Function architecture before re-requesting review.

[dimoschi]

PR Review Summary

Overall Assessment: Request changes 🔄

What this PR does: Adds a parental-consent / minor-report flow, /flag_minor (MOD+), a persistent review-channel UI (approve-ban / deny / recheck-consent), /minor_reviewers admin management with a 60s cache, Nexus-backed consent checks, age-based minor-role assignment/cleanup, and threads ban_id back through SimpleResponse.

Key findings: The feature is thoughtfully built, the persistent-view-by-message-id pattern is correct, the ban_id plumbing through _create_ban_response (including the existing-ban path) is right, and there's a lot of unit coverage. But there's one blocking correctness bug: the two config values the feature depends on (VERIFIED_MINOR role, MINOR_REVIEW channel) are never declared as settings fields, so assign_minor_role and auto_remove_minor_role will AttributeError and flag_minor always reports "not configured." This passed CI only because every test mocks settings, there's no integration test exercising real settings. Beyond that: INFO-level logging of minors' consent data (privacy/GDPR concern), an unbounded reprocessing loop in auto_remove_minor_role, and an untested ctx.respond().edit() pattern that may throw at runtime. See inline comments.

Please also note main is moving to a Pydantic v2 config (nested settings classes with extra="forbid"); this branch will need a coordinated rebase so the new fields/env vars land correctly.

Blocking before merge: the config-field fix (+ a settings-level integration smoke test). The other majors (consent-data logging, reprocessing loop, respond/edit) should be addressed or explicitly justified.

Stats: 33 files changed; 1 critical, 4 major, 2 minor.

[dimoschi]

This PR adds ROLE_VERIFIED_MINOR / CHANNEL_MINOR_REVIEW to .test.env and NEXUS_API_* here, but never declares VERIFIED_MINOR on Roles or MINOR_REVIEW on Channels. Pydantic only populates declared fields, an env var with no matching field is ignored and the attribute simply doesn't exist. Consequences:

• assign_minor_role (minor_verification.py) and auto_remove_minor_role (scheduled_tasks.py) read settings.roles.VERIFIED_MINOR directly → AttributeError at runtime.
• flag_minor uses getattr(..., None) so it won't crash, it'll just always say "Minor review is not configured" and do nothing.

Every test mocks settings, which is why CI is green while the feature is broken end-to-end. Please add the fields:

# Roles
VERIFIED_MINOR: Optional[int] = None
# Channels
MINOR_REVIEW: int = 0

Note: main is heading to a Pydantic v2 config (nested RolesSettings/ChannelsSettings with extra="forbid"). Please coordinate the rebase so these fields land on the nested classes and the new env vars don't trip extra="forbid". An integration test that builds the real Global() settings (not a mock) would have caught this and would prevent regressions.

[dimoschi]

Same root cause as the config.py comment: settings.roles.VERIFIED_MINOR raises AttributeError here because the field is never declared in Roles. auto_remove_minor_role in scheduled_tasks.py hits the same crash. Fix is to declare VERIFIED_MINOR on the settings class.

[dimoschi]

await ctx.respond(...) then status_message.edit(...).** There's no precedent in src/cmds/ for capturing ctx.respond() and calling .edit() on it. In py-cord the initial ApplicationContext.respond() returns an Interaction, whose editor is edit_original_response(), not .edit(). The tests mock ctx.respond as an AsyncMock, so .edit() is never exercised. Please confirm this works against the pinned py-cord version, or switch to interaction.edit_original_response(). As written the happy-path status updates may throw.

[ToxicBiohazard]

@dimoschi Thanks for the thorough reviews — addressed everything in eda791a (includes merge with main). Summary by thread:

Earlier review (Mar)

• HMAC / Cloud Function — replaced with Nexus Bearer auth (f8e4f4f); no HMAC path remains.
• Ban ID race — ban_member_with_epoch returns ban_id; approve modal uses response.ban_id directly.
• Double status write on recheck — consolidated to a single DB write in _recheck_callback.
• get_member_or_user / .roles — isinstance(member, Member) guard before role access in auto_remove_minor_role.
• Leap-year drift — relativedelta for 18th-birthday expiry (scheduled tasks + calculate_ban_duration).
• associated_ban_id FK — c3f1a2b4d5e6 adds ON DELETE SET NULL.
• __tablename__ — explicit on both models.

Latest review (Jun)

• Missing VERIFIED_MINOR / MINOR_REVIEW config fields — declared on nested RolesSettings / ChannelsSettings; .test.env updated to ROLE__* / CHANNEL__* format. Added test_minor_review_settings_load_from_env against real Global().
• Nexus consent INFO logging — dropped to debug, logs status + parsed exists boolean only (no response body).
• auto_remove_minor_role reprocessing loop — query limited to CONSENT_VERIFIED only; transitions to AGED_OUT after cleanup (or when member left / role already gone).
• ctx.respond().edit() — switched to await ctx.edit(...) on ApplicationContext.
• Inverted status colors — added comments clarifying semantics (red = ban approved, green = denied).
• Hardcoded seed IDs + /minor_reviewers seed — removed slash seed command and DEFAULT_REVIEWER_IDS from source; initial reviewers seeded via Alembic d4e5f6a7b8c9 (INSERT IGNORE). add / remove / list kept for runtime management.

Merged main (Pydantic v2 nested settings) — conflicts resolved in config.py, .test.env, pyproject.toml, uv.lock. All tests green locally.

Ready for another look when you have a moment.