feat: bulk image audit v2 — dynamic image types, HQ URL fallback, task tracking, audit of audits

[theism]

Summary

• Dynamic image types: Image questions are now discovered from the CommCare HQ app definition at runtime via OpportunityImageQuestionsAPIView + hq_app_utils.extract_image_questions. Config page shows available image types with form context; default is deselected.
• Fix image display: DataBindOnly questions in the HQ API return empty value fields, so hq_url_path was always blank. Added Strategy 2 fallback: build HQ attachment URL directly from the filename stored at image_path in form JSON (/a/{domain}/api/form/attachment/{xform_id}/{filename}).
• Multi-select image types: Changed _filter_visits_by_related_fields from AND to OR logic — a visit is included if it has any of the selected image types.
• Task tracking: Task creation now persists per-FLW task IDs in run state (flw_tasks dict). Task button turns red/link after creation; state is restored on page reopen. Fixed update_state_api call to use { state: {...} } wrapper.
• Audit of Audits admin report: Org-scoped config phase with multi-select form; period normalization and run links.
• Workflow run list: Added Threshold, % Sampled, Images Reviewed, Tasks Created columns.
• DEBUG-mode live render code: In local development, render_code is served directly from the template Python file — no manual sync step needed.
• Per-FLW sampling: Sampling now ensures equal FLW representation at the configured percentage before shuffling.
• Bulk assessment UI: Incomplete button, scoped Pass All / Fail All / Clear All, task modal (no backdrop), images_reviewed saved to run state.

Test Plan

• Create a Bulk Image Audit workflow run — verify image types are populated from the HQ app
• Select multiple image types — verify bulk assessment shows visits with any of those types
• Verify images display (not broken) for both HQ-hosted URLs and Connect blob images
• Create a task for an FLW, reopen the audit — verify Task button shows red with edit link
• Check workflow run list shows threshold, % sampled, images reviewed, tasks created
• Run pytest commcare_connect/audit/ — all tests pass

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Bulk Image Audit workflow for multi-opportunity image review with sampling, thresholds, per-FLW task creation, and image-type filtering
  • Dynamic image-type detection endpoint and CommCare HQ image proxy
  • Audit of Audits admin report with cross-organization aggregation

• Bug Fixes

  • More robust image parsing, relaxed visit cache validation, timezone-aware timestamps, and more accurate visit_date derivation

• Improvements

  • FLW-centric metrics and UI, Dimagi-gated labs features, and updated template names/descriptions

• Tests / Docs

  • New unit tests for HQ app utilities and updated design/implementation docs

[coderabbitai]

Warning

Rate limit exceeded

@theism has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 1 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4e1f1f6a-bfad-455c-a30e-dde7523eb02e

📥 Commits

Reviewing files that changed from the base of the PR and between b610a57 and 30a427b.

📒 Files selected for processing (2)

• commcare_connect/labs/admin_boundaries/migrations/0002_adminboundary_unique_source_boundary_id.py
• commcare_connect/workflow/views.py

📝 Walkthrough

Walkthrough

Adds a Bulk Image Audit workflow with dynamic image-type discovery and HQ proxy, extensive audit image extraction and HQ app utilities, an Audit of Audits cross-org admin report, Dimagi-gated labs UI items, migration consolidation for admin boundaries, and supporting tests, templates, and docs.

Changes

Cohort / File(s)	Summary
Bulk Image Audit Template & Registry commcare_connect/workflow/templates/bulk_image_audit.py, commcare_connect/workflow/templates/__init__.py, docs/plans/*, docs/superpowers/specs/*	Adds a new bulk_image_audit template (DEFINITION, RENDER_CODE, TEMPLATE), registers it in template exports, and updates design/implementation docs/specs for dynamic image types.
Image-question extraction & HQ proxy commcare_connect/audit/hq_app_utils.py, commcare_connect/audit/views.py, commcare_connect/audit/urls.py, commcare_connect/audit/tests/test_hq_app_utils.py, commcare_connect/audit/tests/test_image_questions_view.py	New utility to extract Image questions (with auto-detected hq_url_path), new GET /audit/api/opportunity/<opp_id>/image-questions/ endpoint and CommCareHQImageProxyView, plus unit/integration tests.
Audit data access & image extraction commcare_connect/audit/data_access.py, commcare_connect/audit/analysis_config.py, commcare_connect/audit/debug_image_fields.py	Refactors image extraction/filtering to support hq_url_path, OR/AND filter rules, per-username sampling, timezone-aware timestamps, richer image metadata (hq_url, xform_id), and adds a diagnostic script for image-field debugging.
Bulk assessment UI & workflow integration commcare_connect/templates/audit/bulk_assessment.html, commcare_connect/templates/workflow/list.html, commcare_connect/workflow/views.py, commcare_connect/workflow/data_access.py, commcare_connect/workflow/templates/audit_with_ai_review.py	Revamps bulk assessment to FLW-level summaries, image-type filtering, task creation modal, read-only handling; adjusts workflow context, render_code sourcing, and renames template text (Weekly → Weekly KMC).
Audit of Audits admin report commcare_connect/custom_analysis/audit_of_audits/data_access.py, commcare_connect/custom_analysis/audit_of_audits/views.py, commcare_connect/custom_analysis/audit_of_audits/urls.py, commcare_connect/templates/custom_analysis/audit_of_audits/*.html	New AuditOfAuditsDataAccess with multi-phase concurrent fetch (sessions, definitions, runs), view with Dimagi gating and config/report modes, and admin config/report templates.
Labs backend parsing, caching, CLI commcare_connect/labs/analysis/backends/csv_parsing.py, commcare_connect/labs/analysis/backends/sql/backend.py, commcare_connect/labs/management/commands/get_cli_token.py	Adds JSON-first image parsing, relaxes cache validity handling when expected counts are unknown, and allows CLI client_secret via settings.
Admin boundaries migration consolidation commcare_connect/labs/admin_boundaries/migrations/0001_initial.py, (deleted: .../0002_*, 0003_*, 0004_*, 0005_*)	Consolidates admin boundary schema changes into an updated initial migration (adds source, removes parent_name, renames table) and removes now-redundant migration files.
Labs UI gating & custom analysis exposure commcare_connect/labs/views.py, config/settings/base.py, config/urls.py	Adds LABS_ADMIN_USERNAMES setting and Dimagi-only gating to surface additional labs projects/custom-analysis items; registers audit_of_audits URL.
Tasks API response & templates commcare_connect/tasks/views.py, commcare_connect/templates/audit/bulk_assessment.html	task_bulk_create now returns created task list; UI integrates task creation/persistence with workflow runs.
Migrations & minor model changes commcare_connect/opportunity/migrations/0115_alter_uservisit_location.py, commcare_connect/program/migrations/0013_alter_program_description.py, (deleted: commcare_connect/solicitations/migrations/0001_add_admin_boundary.py)	Adds migrations altering UserVisit.location and Program.description; removes an old solicitations migration file.
Dev tooling & docs .claude/AGENTS.md, .claude/launch.json, requirements-dev.txt	Adds Audit API Contracts and Workflow Template Anatomy docs, adds a celery-beat launch config, and simplifies dev requirements to include requirements/labs.txt.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UI as Bulk Audit UI
    participant AuditAPI as Audit API
    participant HQ as CommCare HQ
    participant DB as Database

    User->>UI: Open Bulk Image Audit
    UI->>AuditAPI: GET /audit/api/opportunity/<opp_id>/image-questions/
    AuditAPI->>HQ: Fetch app definition
    HQ-->>AuditAPI: App JSON
    AuditAPI->>AuditAPI: extract_image_questions()
    AuditAPI-->>UI: [{id,label,path,hq_url_path}]
    UI->>AuditAPI: POST /audit/creation (related_fields)
    AuditAPI->>DB: Query visits filtered by related_fields
    DB-->>AuditAPI: Visits + attachments
    AuditAPI-->>UI: Created audit session id / streaming progress
    UI->>UI: Stream progress, review images
    User->>UI: Complete review
    UI->>AuditAPI: POST /audit/complete with metrics
    AuditAPI->>DB: Persist run state / metrics
    AuditAPI-->>UI: Redirect / final state

Loading

sequenceDiagram
    participant Admin
    participant ConfigUI
    participant ReportAPI as AuditOfAudits API
    participant LabsAPI as Labs Records API
    participant DB as Labs DB

    Admin->>ConfigUI: Configure orgs & templates, Run
    ConfigUI->>ReportAPI: Request report (org_ids, template_types)
    par Phase 1: Sessions
        ReportAPI->>LabsAPI: GET /sessions per org (concurrent)
        LabsAPI->>DB: Query AuditSession
        DB-->>LabsAPI: Sessions
    and Phase 2A: Definitions
        ReportAPI->>LabsAPI: GET /definitions for opp_ids
        LabsAPI->>DB: Query WorkflowDefinition
        DB-->>LabsAPI: Definitions
    and Phase 2B: Runs
        ReportAPI->>LabsAPI: GET /runs for matching opps (concurrent)
        LabsAPI->>DB: Query WorkflowRun
        DB-->>LabsAPI: Runs
    end
    ReportAPI->>ReportAPI: Join + normalize + compute metrics
    ReportAPI-->>ConfigUI: Aggregated report rows
    ConfigUI->>Admin: Render report table

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

• feat: E2E testing infrastructure with Playwright #34: Overlaps on visit-fetching and cache/streaming logic (changes to fetch_raw_visits/stream_raw_visits).
• Labs mbw workflow v2 #22: Related by adding/registering workflow templates and template registry updates (mbw_monitoring and template plumbing).
• feat: MBW Monitoring Dashboard v3 — GPS, streaming, performance & approval filters #33: Touches labs analysis backend caching and filtering logic similar to CSV/SQL backend edits here.

Poem

🐇 I hopped through apps and HQ trees,

I found the image questions with ease,
stitched audits across many lands,
chased blobs and matched filenames with hands.
Little workflows bloom—rabbit cheers and carrots please!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title clearly summarizes the main changes: bulk image audit v2 with dynamic image types, HQ URL fallback, task tracking, and audit of audits report—all of which are substantively present in the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch labs-auditv2

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 15

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

commcare_connect/audit/data_access.py (1)

866-893: ⚠️ Potential issue | 🟠 Major

The field filter still turns multi-select back into an effective AND.

After the image OR check, the code requires every filter_by_field rule to match. A visit that satisfies one selected image type plus its field value, but not the other selected types, still gets dropped.

Suggested fix

-            # AND logic: visit must satisfy every field filter rule
-            if include_visit:
-                for rule in field_filter_rules:
-                    field_path = rule.get("field_path", "")
-                    has_field_value = False
-                    for img in images:
-                        for rf in img.get("related_fields", []):
-                            if rf.get("path") == field_path and rf.get("value"):
-                                has_field_value = True
-                                break
-                        if has_field_value:
-                            break
-                    if not has_field_value:
-                        include_visit = False
-                        break
+            # OR across the matched image types: a visit can qualify through any selected rule
+            if include_visit and field_filter_rules:
+                question_ids = {img.get("question_id") for img in images}
+                applicable_rules = [r for r in field_filter_rules if r.get("image_path") in question_ids]
+                if not applicable_rules:
+                    include_visit = False
+                else:
+                    include_visit = any(
+                        any(
+                            rf.get("path") == rule.get("field_path") and rf.get("value")
+                            for img in images
+                            for rf in img.get("related_fields", [])
+                        )
+                        for rule in applicable_rules
+                    )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/audit/data_access.py` around lines 866 - 893, The current
logic sets include_visit False unless every rule in field_filter_rules matches
(AND), which negates the intended multi-select (OR) behavior; change the
field-filter check so a visit is included if any rule in field_filter_rules
matches (i.e., replace the per-rule all-match loop with an any(...) check).
Specifically, in the visit loop over visit_images use a boolean like
rule_matched = any(there exists an img in images with a related_fields entry
where rf.get("path")==rule.get("field_path") and rf.get("value") for each rule)
— then set include_visit = include_visit and (not field_filter_rules or
rule_matched) (or equivalent) so that image_filter_paths and field_filter_rules
combine via OR semantics rather than forcing all field rules to pass; update
references to image_filter_paths, field_filter_rules, visit_images, field_path
and related_fields accordingly.

commcare_connect/workflow/views.py (1)

359-387: ⚠️ Potential issue | 🟠 Major

Don't create a fallback run here and then return an error anyway.

If get() falls through after a creation failure, this branch creates a second run and then immediately returns an error page. That leaves an orphaned run behind while still surfacing the failure state.

Suggested fix

             else:
-                # Create new run (always creates a fresh run)
-                from datetime import datetime, timedelta, timezone
-
-                today = datetime.now(timezone.utc).date()
-                week_start = today - timedelta(days=today.weekday())
-                week_end = week_start + timedelta(days=6)
-
-                run = data_access.create_run(
-                    definition_id=definition_id,
-                    opportunity_id=opportunity_id,
-                    period_start=week_start.isoformat(),
-                    period_end=week_end.isoformat(),
-                    initial_state={"worker_states": {}},
-                )
-                run_data = {
-                    "id": run.id,
-                    "definition_id": definition_id,
-                    "opportunity_id": opportunity_id,
-                    "opportunity_name": labs_context.get("opportunity", {}).get("name"),
-                    "status": run.data.get("status", "in_progress"),
-                    "state": run.data.get("state", {}),
-                    "period_start": run.data.get("period_start"),
-                    "period_end": run.data.get("period_end"),
-                }
-                context["is_edit_mode"] = False
-                # No run_id and not edit mode — get() should have redirected.
-                # This branch only executes if opportunity_id was missing at
-                # get() time (no labs context), so show a friendly error.
-                context["error"] = "Could not create a new run. Please select an opportunity."
+                context["error"] = "Could not create a new run. Please refresh and try again."
                 return context

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/workflow/views.py` around lines 359 - 387, The code is
creating a new run even when opportunity_id (labs_context) is missing and then
immediately setting context["error"], which leaves an orphaned run; modify the
get() branch so you only call data_access.create_run when opportunity_id is
present and valid, and handle create_run failures by catching exceptions or
checking its return before constructing run_data; ensure you remove the
unconditional call to create_run in this branch and instead set context["error"]
(and avoid creating run_data) when opportunity_id is absent or create_run fails,
referencing create_run, run_data, and context["error"] to locate and update the
logic.

🟡 Minor comments (9)

commcare_connect/workflow/templates/audit_with_ai_review.py-13-14 (1)

13-14: ⚠️ Potential issue | 🟡 Minor

Keep the template copy consistent about AI being optional.

DEFINITION.description says AI review is optional, but TEMPLATE.description still reads as mandatory. That will surface conflicting copy between the template picker and the run page.

Suggested fix

-    "description": "Create weekly KMC audit sessions per FLW with AI image validation",
+    "description": "Create weekly KMC audit sessions per FLW with optional AI image validation",

Also applies to: 989-990

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/workflow/templates/audit_with_ai_review.py` around lines 13
- 14, DEFINITION.description indicates AI review is optional but
TEMPLATE.description currently implies it's mandatory; update the TEMPLATE
dictionary's "description" value (the entry alongside "name": "Weekly KMC Audit
with AI Review") to match the optional wording used in DEFINITION.description so
both copies are consistent across TEMPLATE and DEFINITION (also apply the same
change for the other occurrence around lines noted).

commcare_connect/templates/workflow/list.html-803-815 (1)

803-815: ⚠️ Potential issue | 🟡 Minor

Render all selected image types here.

This block still assumes a single scalar image_type, but bulk image audit now supports multi-select image types. Runs with multiple selections will either show a raw list/string here or lose the friendly badge mapping entirely. Normalize the stored value to a list and render one badge per type, with a string fallback for older runs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/templates/workflow/list.html` around lines 803 - 815, The
template assumes run.state.config.image_type is a single scalar; update the
rendering to normalize image_type into a list (e.g., treat a string or
comma/JSON value as a single-item list, and handle already-array values) and
loop over it to render one badge per type using the existing mapping for
"ors_photo", "muac_photo", "scale_photo" (refer to workflow.template_type and
run.state.config.image_type), falling back to outputting the raw string when
normalization fails for older runs; ensure the {% with %} block is replaced by a
normalized variable and a {% for img in images %} loop so multiple selected
image types each get their own badge.

docs/plans/2026-02-25-bulk-image-audit-design.md-10-12 (1)

10-12: ⚠️ Potential issue | 🟡 Minor

Refresh this design doc to match the shipped v2 flow.

This still documents a single fixed image type and says there are no commcare_connect/audit/ view or URL changes, but the implementation in this PR now discovers image questions at runtime, supports multi-select image filters, and adds new audit endpoints/views. Leaving the plan in this state will mislead follow-up work and QA.

Also applies to: 21-23, 37-68, 84-121

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/plans/2026-02-25-bulk-image-audit-design.md` around lines 10 - 12,
Update the design doc to reflect the shipped v2 implementation: change
descriptions to note that image questions are discovered at runtime (not a
single fixed image type), support for multi-select image filters is included,
and new audit endpoints/views under commcare_connect/audit/ exist (replace the
claim of no URL/view changes); rename the template mention to "Weekly KMC Audit
with AI Review" and update any workflow/UI flow diagrams, API sections, and QA
steps (references to "Bulk Image Audit" template, runtime image discovery,
multi-select filters, and commcare_connect/audit/ endpoints should be corrected
across the sections you flagged: lines roughly 21-23, 37-68, and 84-121).

commcare_connect/custom_analysis/audit_of_audits/views.py-158-160 (1)

158-160: ⚠️ Potential issue | 🟡 Minor

Keep the applied template_types subset visible in the report UI.

selected_template_types is already parsed above, but the report state later reads template_type instead. After the config page submits a template_types filter, the report renders as if no template subset was applied.

Also applies to: 220-240

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/custom_analysis/audit_of_audits/views.py` around lines 158 -
160, The report is reading the wrong parameter name so the selected template
subset is lost; change usages that read "template_type" to use the parsed list
"selected_template_types" (and the derived "template_types_filter") so the
report state and rendered context preserve the applied list filter. Locate the
view code that builds the report state/context (references:
selected_template_types, template_types_filter) and replace any occurrences of
"template_type" with the list-valued "template_types" or pass
selected_template_types into the context; apply the same fix to the later block
mentioned (around the 220-240 area) so both initial parsing and final rendering
use the same variable names.

commcare_connect/audit/views.py-1851-1859 (1)

1851-1859: ⚠️ Potential issue | 🟡 Minor

Return 401 before calling Connect when the Labs OAuth token is missing.

Right now an expired or missing session token falls into the generic metadata-fetch exception path and comes back as 502, which makes auth expiry look like an upstream outage.

Suggested fix

     def get(self, request, opp_id: int):
         labs_oauth = request.session.get("labs_oauth", {})
         access_token = labs_oauth.get("access_token", "")
+        if not access_token:
+            return JsonResponse({"error": "Not authenticated"}, status=401)
 
         # Step 1: Resolve cc_domain + cc_app_id from Connect
         try:
             meta = fetch_opportunity_metadata(access_token, opp_id)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/audit/views.py` around lines 1851 - 1859, Check for a
missing/empty Labs OAuth token in the session (labs_oauth/access_token) before
calling fetch_opportunity_metadata(opp_id); if access_token is absent or blank,
immediately return a JsonResponse with a 401 status and an appropriate error
body instead of attempting fetch_opportunity_metadata so expired/missing auth is
distinguished from upstream errors logged by the fetch_opportunity_metadata
exception handler.

commcare_connect/workflow/templates/bulk_image_audit.py-743-747 (1)

743-747: ⚠️ Potential issue | 🟡 Minor

Show the human image-type label here.

The image-question API returns label for user-facing display, but this card renders q.id, which exposes internal question IDs instead of recognizable image types. That makes the new dynamic selector much harder to use.

Suggested fix

-                                            <div className="text-sm font-medium text-gray-900 font-mono">{q.id}</div>
+                                            <div className="text-sm font-medium text-gray-900">
+                                                {q.label || q.id}
+                                            </div>
+                                            {q.label && q.id && (
+                                                <div className="text-xs text-gray-400 mt-0.5 font-mono">
+                                                    {q.id}
+                                                </div>
+                                            )}
                                             {q.form_name && (
                                                 <div className="text-xs text-gray-500 mt-0.5">{q.form_name}</div>
                                             )}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/workflow/templates/bulk_image_audit.py` around lines 743 -
747, Replace the internal question id shown in the card with the human-facing
image-type label from the API: use q.label for the main display instead of q.id
(and keep q.form_name as-is); if q.label may be absent, render q.label || q.id
so you still have a fallback. Update the JSX inside the <div className="text-sm
font-medium text-gray-900 font-mono"> to reference q.label (with fallback) so
users see the readable image-type name.

commcare_connect/templates/custom_analysis/audit_of_audits/report.html-175-185 (1)

175-185: ⚠️ Potential issue | 🟡 Minor

Preserve zero metrics in the row data-* attributes.

default:'' turns legitimate 0 values into empty strings here, so runs with 0%, 0 images, or 0 tasks sort like nulls even though the visible cell shows 0. Use default_if_none or an explicit is not None guard for these sort keys.

Suggested fix

-              data-pct-sampled="{{ row.pct_sampled|default:'' }}"
-              data-avg-passed="{{ row.avg_pct_passed|default:'' }}"
-              data-passing="{{ row.pct_passing|default:'' }}"
-              data-images="{{ row.images_reviewed|default:'' }}"
-              data-tasks="{{ row.tasks_created|default:'' }}"
+              data-pct-sampled="{{ row.pct_sampled|default_if_none:'' }}"
+              data-avg-passed="{{ row.avg_pct_passed|default_if_none:'' }}"
+              data-passing="{{ row.pct_passing|default_if_none:'' }}"
+              data-images="{{ row.images_reviewed|default_if_none:'' }}"
+              data-tasks="{{ row.tasks_created|default_if_none:'' }}"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/templates/custom_analysis/audit_of_audits/report.html`
around lines 175 - 185, The data-* attributes (e.g., data-flws for
row.selected_count, data-pct-sampled for row.pct_sampled, data-images for
row.images_reviewed, data-tasks for row.tasks_created, etc.) use default:''
which converts legitimate zero values to empty strings; change these to preserve
zeros by using default_if_none (or an explicit check like "if value is not None
else ''") for each attribute so that 0 remains "0" in the rendered data
attributes and sorting behaves correctly.

docs/superpowers/specs/2026-03-10-bulk-image-audit-dynamic-image-types-design.md-76-83 (1)

76-83: ⚠️ Potential issue | 🟡 Minor

Spec still documents image types as default-selected.

These lines say every discovered image type is selected on load, but the current workflow behavior/test plan expects the config to start with all image types deselected. Please align the spec so QA and follow-up changes are not validating the opposite behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@docs/superpowers/specs/2026-03-10-bulk-image-audit-dynamic-image-types-design.md`
around lines 76 - 83, The spec currently states that all discovered image types
are default-selected on load (see imageQuestions and selectedImageTypeIds), but
QA expects them to start deselected; update the spec to reflect that
selectedImageTypeIds should default to an empty array after fetching
`/audit/api/opportunity/{instance.opportunity_id}/image-questions/` (and not
“Default-select all returned types”), and ensure the On mount section clarifies
fetching imageQuestions populates imageQuestions and sets selectedImageTypeIds =
[] with imageQuestionsLoading/imageQuestionsError behavior unchanged.

docs/plans/2026-02-25-bulk-image-audit-implementation.md-7-10 (1)

7-10: ⚠️ Potential issue | 🟡 Minor

This plan no longer matches the implementation.

It says the template uses only pre-existing audit endpoints and needs no backend work, but this PR adds HQ-backed image-question discovery/proxy endpoints. If this doc is meant to stay as a reference, please update it or mark it historical so follow-up work is not sent down the wrong path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/plans/2026-02-25-bulk-image-audit-implementation.md` around lines 7 -
10, The documentation in this plan incorrectly states that the new template
mirrors audit_with_ai_review.py and uses only pre-existing audit endpoints;
update the plan text to reflect that the PR adds new HQ-backed image-question
discovery/proxy endpoints (or mark the doc as historical/archived to avoid
future confusion). Specifically, update the Architecture and Tech Stack
paragraphs to reference the new
commcare_connect/workflow/templates/bulk_image_audit.py behavior and the added
HQ-backed endpoints (or add a clear “historical” banner), and ensure the
TEMPLATE and render_code references and any mentions of audit_with_ai_review.py
accurately note the divergence from pre-existing-only endpoints.

🧹 Nitpick comments (2)

commcare_connect/tasks/views.py (1)

463-463: Minor: Use f-string conversion flag for cleaner formatting.

Per Ruff RUF010, prefer {e!s} over {str(e)} in f-strings for explicit conversion.

Suggested fix

-                errors.append(f"Failed to create task for FLW {flw_id}: {str(e)}")
+                errors.append(f"Failed to create task for FLW {flw_id}: {e!s}")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/tasks/views.py` at line 463, Replace the explicit str()
conversion inside the f-string used when appending errors so it uses the
f-string conversion flag; update the errors.append call that currently reads
errors.append(f"Failed to create task for FLW {flw_id}: {str(e)}") to use {e!s}
(referencing the errors list, flw_id variable and exception variable e) for
cleaner formatting and to satisfy Ruff RUF010.

config/settings/base.py (1)

461-467: Move the local-admin fallback into config.settings.local.

The inline doc says this allowlist is only for local development, but defining it in base.py makes it available in every environment. Keeping it in config.settings.local (or behind an explicit dev-only flag) would make that constraint enforceable instead of relying on deployment discipline.

As per coding guidelines, Use config.settings.local for local development. Do not use config.settings.labs_aws for local development—it is only for AWS deployment at labs.connect.dimagi.com.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@config/settings/base.py` around lines 461 - 467, The LABS_ADMIN_USERNAMES
allowlist is defined in base settings but is meant for local development only;
move the LABS_ADMIN_USERNAMES = env.list("LABS_ADMIN_USERNAMES", default=[])
declaration out of config.settings.base (where it currently lives) into
config.settings.local so it only applies in dev, or alternatively guard it
behind a dev-only flag (e.g., check DEBUG or a new DEV_ONLY setting) and
reference the same symbol LABS_ADMIN_USERNAMES so other code continues to import
it unchanged; update any imports or settings loading so config.settings.local
provides this value in local runs and remove the dev-only fallback from base.py.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@commcare_connect/audit/data_access.py`:
- Around line 781-829: The code currently skips processing HQ/DataBindOnly
fallbacks for a visit as soon as any Connect blob exists (the "if images:
continue"), causing missing HQ-only image types to be skipped; remove that early
continue and instead, inside the loop over image_rules, check per-rule whether
that specific image type is already present on the visit (e.g., compute the
target key like image_path or derived name and do exists =
any(img.get("question_id")==image_path or img.get("name")==name for img in
images)); only add the HQ fallback image when not exists. Keep the rest of the
logic (hq_url construction, blob_id creation, use of self._extract_field_value,
visit_data fields) the same and append the new dict to images when missing.

In `@commcare_connect/audit/hq_app_utils.py`:
- Around line 61-64: The current ID selection uses leaf_id then image_path,
which can still collide for identical XPath across forms; update the logic in
the loop that computes leaf_id = _question_id(xform_path) and question_id to add
a third fallback using a form-stable discriminator (e.g., include the form
xmlns) when image_path is already in seen_ids — for example, if leaf_id in
seen_ids then try image_path, and if image_path in seen_ids then use a composite
like f"{xmlns}:{image_path}" (or similar unique concatenation), then add that
final question_id to seen_ids; also add a regression test that constructs two
forms with the same question path (e.g., /data/group/photo) but different xmlns
and asserts produced IDs are distinct.

In `@commcare_connect/audit/views.py`:
- Around line 689-723: The view currently trusts a client-supplied hq_url and
uses server COMMCARE_API_KEY/COMMCARE_USERNAME to fetch it; change it to accept
a server-side attachment identifier (e.g., attachment_id or blob_id) instead of
url, resolve the actual CommCare attachment path on the backend from the
audit/session/blob model, verify the current user's permission/domain matches
the attachment's allowed scope, then construct the trusted hq_url from the known
COMMCARE_HQ_URL plus the resolved path and call httpx.get (keeping existing
headers/timeout) — remove/disable using request.GET["url"] and the loose
host/path checks and ensure permission checks occur before any external fetch.

In `@commcare_connect/custom_analysis/audit_of_audits/data_access.py`:
- Around line 240-247: Currently the code only records opp IDs with any matching
definitions (opps_with_matching_defs) after calling
self._fetch_definitions_for_opp, which still allows unrelated runs from that opp
to be emitted; change opps_with_matching_defs from a set to a mapping of opp_id
-> set(matching_definition_ids) by collecting d.id for each definition d whose
d.template_type matches self.template_types when building def_map in the
ThreadPoolExecutor block (the block that calls self._fetch_definitions_for_opp
and updates def_map/d). Then update the row-building logic that emits runs to
consult that mapping and only emit runs whose run.definition_id is in the
matching_definition_ids for that opp (i.e., filter by definition IDs, not just
opportunity IDs). Apply the same change to the corresponding logic used in the
other occurrence mentioned (the later block that currently uses
opps_with_matching_defs).

In `@commcare_connect/custom_analysis/audit_of_audits/views.py`:
- Around line 162-168: The current building of opportunity_ids and opp_name_map
pulls every opportunity from user_opps and sends them into
AuditOfAuditsDataAccess even when an org filter is selected; update the
comprehensions that build opportunity_ids and opp_name_map to first filter
user_opps by the selected organization(s) (e.g., check o.get("organization_id")
or similar against the selected_org_ids/selected_org variable in scope) so only
opportunities belonging to the selected org(s) are included, and make the same
change in the other occurrence around lines 192-197 where opportunities are
assembled; ensure the filtered opportunity_ids/opp_name_map are what's passed
into AuditOfAuditsDataAccess and any downstream report construction.

In `@commcare_connect/labs/admin_boundaries/migrations/0001_initial.py`:
- Around line 33-35: The migration defines models.CharField "boundary_id" with
unique=True which enforces global uniqueness and will collide across different
sources; change the field to remove unique=True and instead add a composite
unique constraint on (source, boundary_id) for the model (e.g., add a
UniqueConstraint on fields ["source", "boundary_id"] in the model's Meta or
constraints section of the migration for the AdminBoundary model) so IDs remain
unique per source rather than globally.

In `@commcare_connect/labs/analysis/backends/sql/backend.py`:
- Around line 175-178: Do not coerce expected_visit_count to 0; preserve None as
the unknown sentinel. Replace the line setting effective_count =
expected_visit_count or 0 with effective_count = expected_visit_count so None
stays distinct from 0, and call
cache_manager.has_valid_raw_cache(effective_count, tolerance_pct=tolerance_pct)
(or, if has_valid_raw_cache does not accept None, only pass tolerance_pct when
expected_visit_count is not None). This keeps a real zero visit count from being
treated as the "unknown" case in the has_valid_raw_cache check.
- Around line 116-121: The current cache hit logic (involving force_refresh,
expected_visit_count and cache_manager.has_valid_raw_cache) can return a
non-image raw cache for requests with include_images=True; update the condition
so image requests only reuse caches known to include images. Concretely, when
evaluating the if-not-force_refresh branch, either pass an image-aware flag into
cache_manager.has_valid_raw_cache (e.g., has_valid_raw_cache(effective_count,
tolerance_pct=tolerance_pct, include_images=include_images)) or add an explicit
check: if include_images and not cache_manager.cache_has_images(): skip cache
reuse (force a refresh); keep the existing effective_count/tolerance logic for
non-image requests.

In `@commcare_connect/opportunity/migrations/0115_alter_uservisit_location.py`:
- Around line 12-16: This migration alters opportunity.UserVisit.location in a
labs-only change; revert/remove the AlterField migration in
0115_alter_uservisit_location.py so you do not ship schema changes under
commcare_connect/opportunity, and instead implement the labs-only behavior in
the labs storage layer (e.g., add handling in the labs storage adapter or the
labs app code that reads/writes the UserVisit.location field without changing
the ORM model); specifically, remove or skip the AlterField targeting
UserVisit.location and move any data-migration or storage logic into the
labs-specific module responsible for Bulk Image Audit.

In `@commcare_connect/templates/audit/bulk_assessment.html`:
- Around line 845-852: The current on-load block that POSTs images_reviewed
using workflowRunId and this.allAssessments.length should be removed; do not
persist reviewed count on initial load. Instead, compute images_reviewed as the
sum of reviewed assessments (pass + fail + incomplete) at the moment the user
saves or completes the run and send the POST there (i.e., inside the
save/complete handler that persists the workflow run state), using the same
endpoint and CSRF headers used in the removed fetch call; ensure you reference
and update the images_reviewed field only from that save/complete path.
- Around line 1480-1481: The avgPassed calculation currently uses totalAssessed
= this.stats.pass + this.stats.fail which omits this.stats.incomplete; change
the denominator to include incomplete by computing totalAssessed =
this.stats.pass + this.stats.fail + this.stats.incomplete (or otherwise add
this.stats.incomplete into the denominator) and then compute avgPassed using
this.stats.pass / totalAssessed * 100 (with the same guard for totalAssessed >
0), updating references to totalAssessed and avgPassed accordingly.
- Around line 1099-1104: The code currently proceeds when data.success is true
even if data.tasks[0].id is missing; change the flow in the task creation
handler so you validate that const taskId = data.tasks && data.tasks[0] ?
data.tasks[0].id : null is non-null and, if null, throw an Error (or reject)
instead of updating this.createdTaskFLWs and this.flwTasks; only after
confirming taskId exists should you add this.taskModal.flw.username to
this.createdTaskFLWs and set this.flwTasks[flwUsername] = taskId (preserving the
existing workflowRunId branch).

In `@commcare_connect/templates/workflow/list.html`:
- Around line 875-910: Use a single threshold key (run.state.pass_threshold)
when computing the color for avg: change the with-block to assign
threshold=run.state.pass_threshold|default:80 and use that threshold for the
color check instead of run.state.threshold. Also change the truthy checks that
currently hide zeros (run.state.sample_percentage, run.state.images_reviewed,
run.state.tasks_created) to explicit existence checks (e.g., {% if
run.state.sample_percentage is not none and run.state.sample_percentage != ""
%}) so 0 displays; apply the same explicit None/empty check pattern where
appropriate (avg already checks avg != "" and avg is not None).

In `@commcare_connect/workflow/templates/bulk_image_audit.py`:
- Around line 410-423: The handleComplete function currently only calls
onUpdateState which patches run.data.state; instead call the run-completion
action (the API/method that marks a run complete at top-level, e.g., invoke the
run completion endpoint or action named "run-completion" or completeRun) before
or alongside persisting the metadata, then call onUpdateState to save avg_passed
and completion metadata into run.data.state; update the try block in
handleComplete to first await the run-completion action (passing run id and
completion payload), then await onUpdateState with avg_passed and completion
fields so both the run's top-level status and the run.data.state are correctly
persisted and the redirect will reflect completed status.

In `@commcare_connect/workflow/views.py`:
- Around line 334-335: Replace the places that set run_data["opportunity_name"]
from labs_context.get("opportunity", {}).get("name") with the existing
labs_context["opportunity_name"] (or labs_context.get("opportunity_name")) so
run_data uses the same precomputed opportunity_name value the view reads
earlier; update the run_data assignments (the branches that build run_data) to
reference labs_context["opportunity_name"] instead of accessing the nested
"opportunity" dict (affecting the run_data construction around the symbols
run_data and labs_context and the usage that leads to
instance.opportunity_name).

---

Outside diff comments:
In `@commcare_connect/audit/data_access.py`:
- Around line 866-893: The current logic sets include_visit False unless every
rule in field_filter_rules matches (AND), which negates the intended
multi-select (OR) behavior; change the field-filter check so a visit is included
if any rule in field_filter_rules matches (i.e., replace the per-rule all-match
loop with an any(...) check). Specifically, in the visit loop over visit_images
use a boolean like rule_matched = any(there exists an img in images with a
related_fields entry where rf.get("path")==rule.get("field_path") and
rf.get("value") for each rule) — then set include_visit = include_visit and (not
field_filter_rules or rule_matched) (or equivalent) so that image_filter_paths
and field_filter_rules combine via OR semantics rather than forcing all field
rules to pass; update references to image_filter_paths, field_filter_rules,
visit_images, field_path and related_fields accordingly.

In `@commcare_connect/workflow/views.py`:
- Around line 359-387: The code is creating a new run even when opportunity_id
(labs_context) is missing and then immediately setting context["error"], which
leaves an orphaned run; modify the get() branch so you only call
data_access.create_run when opportunity_id is present and valid, and handle
create_run failures by catching exceptions or checking its return before
constructing run_data; ensure you remove the unconditional call to create_run in
this branch and instead set context["error"] (and avoid creating run_data) when
opportunity_id is absent or create_run fails, referencing create_run, run_data,
and context["error"] to locate and update the logic.

---

Minor comments:
In `@commcare_connect/audit/views.py`:
- Around line 1851-1859: Check for a missing/empty Labs OAuth token in the
session (labs_oauth/access_token) before calling
fetch_opportunity_metadata(opp_id); if access_token is absent or blank,
immediately return a JsonResponse with a 401 status and an appropriate error
body instead of attempting fetch_opportunity_metadata so expired/missing auth is
distinguished from upstream errors logged by the fetch_opportunity_metadata
exception handler.

In `@commcare_connect/custom_analysis/audit_of_audits/views.py`:
- Around line 158-160: The report is reading the wrong parameter name so the
selected template subset is lost; change usages that read "template_type" to use
the parsed list "selected_template_types" (and the derived
"template_types_filter") so the report state and rendered context preserve the
applied list filter. Locate the view code that builds the report state/context
(references: selected_template_types, template_types_filter) and replace any
occurrences of "template_type" with the list-valued "template_types" or pass
selected_template_types into the context; apply the same fix to the later block
mentioned (around the 220-240 area) so both initial parsing and final rendering
use the same variable names.

In `@commcare_connect/templates/custom_analysis/audit_of_audits/report.html`:
- Around line 175-185: The data-* attributes (e.g., data-flws for
row.selected_count, data-pct-sampled for row.pct_sampled, data-images for
row.images_reviewed, data-tasks for row.tasks_created, etc.) use default:''
which converts legitimate zero values to empty strings; change these to preserve
zeros by using default_if_none (or an explicit check like "if value is not None
else ''") for each attribute so that 0 remains "0" in the rendered data
attributes and sorting behaves correctly.

In `@commcare_connect/templates/workflow/list.html`:
- Around line 803-815: The template assumes run.state.config.image_type is a
single scalar; update the rendering to normalize image_type into a list (e.g.,
treat a string or comma/JSON value as a single-item list, and handle
already-array values) and loop over it to render one badge per type using the
existing mapping for "ors_photo", "muac_photo", "scale_photo" (refer to
workflow.template_type and run.state.config.image_type), falling back to
outputting the raw string when normalization fails for older runs; ensure the {%
with %} block is replaced by a normalized variable and a {% for img in images %}
loop so multiple selected image types each get their own badge.

In `@commcare_connect/workflow/templates/audit_with_ai_review.py`:
- Around line 13-14: DEFINITION.description indicates AI review is optional but
TEMPLATE.description currently implies it's mandatory; update the TEMPLATE
dictionary's "description" value (the entry alongside "name": "Weekly KMC Audit
with AI Review") to match the optional wording used in DEFINITION.description so
both copies are consistent across TEMPLATE and DEFINITION (also apply the same
change for the other occurrence around lines noted).

In `@commcare_connect/workflow/templates/bulk_image_audit.py`:
- Around line 743-747: Replace the internal question id shown in the card with
the human-facing image-type label from the API: use q.label for the main display
instead of q.id (and keep q.form_name as-is); if q.label may be absent, render
q.label || q.id so you still have a fallback. Update the JSX inside the <div
className="text-sm font-medium text-gray-900 font-mono"> to reference q.label
(with fallback) so users see the readable image-type name.

In `@docs/plans/2026-02-25-bulk-image-audit-design.md`:
- Around line 10-12: Update the design doc to reflect the shipped v2
implementation: change descriptions to note that image questions are discovered
at runtime (not a single fixed image type), support for multi-select image
filters is included, and new audit endpoints/views under commcare_connect/audit/
exist (replace the claim of no URL/view changes); rename the template mention to
"Weekly KMC Audit with AI Review" and update any workflow/UI flow diagrams, API
sections, and QA steps (references to "Bulk Image Audit" template, runtime image
discovery, multi-select filters, and commcare_connect/audit/ endpoints should be
corrected across the sections you flagged: lines roughly 21-23, 37-68, and
84-121).

In `@docs/plans/2026-02-25-bulk-image-audit-implementation.md`:
- Around line 7-10: The documentation in this plan incorrectly states that the
new template mirrors audit_with_ai_review.py and uses only pre-existing audit
endpoints; update the plan text to reflect that the PR adds new HQ-backed
image-question discovery/proxy endpoints (or mark the doc as historical/archived
to avoid future confusion). Specifically, update the Architecture and Tech Stack
paragraphs to reference the new
commcare_connect/workflow/templates/bulk_image_audit.py behavior and the added
HQ-backed endpoints (or add a clear “historical” banner), and ensure the
TEMPLATE and render_code references and any mentions of audit_with_ai_review.py
accurately note the divergence from pre-existing-only endpoints.

In
`@docs/superpowers/specs/2026-03-10-bulk-image-audit-dynamic-image-types-design.md`:
- Around line 76-83: The spec currently states that all discovered image types
are default-selected on load (see imageQuestions and selectedImageTypeIds), but
QA expects them to start deselected; update the spec to reflect that
selectedImageTypeIds should default to an empty array after fetching
`/audit/api/opportunity/{instance.opportunity_id}/image-questions/` (and not
“Default-select all returned types”), and ensure the On mount section clarifies
fetching imageQuestions populates imageQuestions and sets selectedImageTypeIds =
[] with imageQuestionsLoading/imageQuestionsError behavior unchanged.

---

Nitpick comments:
In `@commcare_connect/tasks/views.py`:
- Line 463: Replace the explicit str() conversion inside the f-string used when
appending errors so it uses the f-string conversion flag; update the
errors.append call that currently reads errors.append(f"Failed to create task
for FLW {flw_id}: {str(e)}") to use {e!s} (referencing the errors list, flw_id
variable and exception variable e) for cleaner formatting and to satisfy Ruff
RUF010.

In `@config/settings/base.py`:
- Around line 461-467: The LABS_ADMIN_USERNAMES allowlist is defined in base
settings but is meant for local development only; move the LABS_ADMIN_USERNAMES
= env.list("LABS_ADMIN_USERNAMES", default=[]) declaration out of
config.settings.base (where it currently lives) into config.settings.local so it
only applies in dev, or alternatively guard it behind a dev-only flag (e.g.,
check DEBUG or a new DEV_ONLY setting) and reference the same symbol
LABS_ADMIN_USERNAMES so other code continues to import it unchanged; update any
imports or settings loading so config.settings.local provides this value in
local runs and remove the dev-only fallback from base.py.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6cf89a84-3261-443d-9674-1e7b705a3715

📥 Commits

Reviewing files that changed from the base of the PR and between 0390261 and 5abb95f.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (43)

• .claude/AGENTS.md
• .claude/launch.json
• commcare_connect/audit/analysis_config.py
• commcare_connect/audit/data_access.py
• commcare_connect/audit/debug_image_fields.py
• commcare_connect/audit/hq_app_utils.py
• commcare_connect/audit/tests/__init__.py
• commcare_connect/audit/tests/test_hq_app_utils.py
• commcare_connect/audit/tests/test_image_questions_view.py
• commcare_connect/audit/urls.py
• commcare_connect/audit/views.py
• commcare_connect/custom_analysis/audit_of_audits/__init__.py
• commcare_connect/custom_analysis/audit_of_audits/data_access.py
• commcare_connect/custom_analysis/audit_of_audits/urls.py
• commcare_connect/custom_analysis/audit_of_audits/views.py
• commcare_connect/labs/admin_boundaries/migrations/0001_initial.py
• commcare_connect/labs/admin_boundaries/migrations/0002_rename_table.py
• commcare_connect/labs/admin_boundaries/migrations/0003_add_source_remove_parent.py
• commcare_connect/labs/admin_boundaries/migrations/0004_add_grid3_hdx_sources.py
• commcare_connect/labs/admin_boundaries/migrations/0005_add_geopode_source.py
• commcare_connect/labs/analysis/backends/csv_parsing.py
• commcare_connect/labs/analysis/backends/sql/backend.py
• commcare_connect/labs/management/commands/get_cli_token.py
• commcare_connect/labs/views.py
• commcare_connect/opportunity/migrations/0115_alter_uservisit_location.py
• commcare_connect/program/migrations/0013_alter_program_description.py
• commcare_connect/solicitations/migrations/0001_add_admin_boundary.py
• commcare_connect/tasks/views.py
• commcare_connect/templates/audit/bulk_assessment.html
• commcare_connect/templates/custom_analysis/audit_of_audits/config.html
• commcare_connect/templates/custom_analysis/audit_of_audits/report.html
• commcare_connect/templates/workflow/list.html
• commcare_connect/workflow/data_access.py
• commcare_connect/workflow/templates/__init__.py
• commcare_connect/workflow/templates/audit_with_ai_review.py
• commcare_connect/workflow/templates/bulk_image_audit.py
• commcare_connect/workflow/views.py
• config/settings/base.py
• config/urls.py
• docs/plans/2026-02-25-bulk-image-audit-design.md
• docs/plans/2026-02-25-bulk-image-audit-implementation.md
• docs/superpowers/specs/2026-03-10-bulk-image-audit-dynamic-image-types-design.md
• requirements-dev.txt

💤 Files with no reviewable changes (5)

• commcare_connect/labs/admin_boundaries/migrations/0004_add_grid3_hdx_sources.py
• commcare_connect/labs/admin_boundaries/migrations/0002_rename_table.py
• commcare_connect/solicitations/migrations/0001_add_admin_boundary.py
• commcare_connect/labs/admin_boundaries/migrations/0003_add_source_remove_parent.py
• commcare_connect/labs/admin_boundaries/migrations/0005_add_geopode_source.py

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Do not proxy arbitrary HQ attachment URLs with shared credentials.

This view accepts a client-supplied url and fetches it with the server's CommCare HQ API key. Any authenticated Labs user who can guess a valid attachment path can use this endpoint to read HQ attachments outside the current audit/session scope. Resolve the attachment location server-side from a trusted session/blob identifier, or validate it against the current user's allowed opportunity/domain before proxying.

🧰 Tools

🪛 Ruff (0.15.5)

[warning] 704-704: Do not catch blind exception: Exception

(BLE001)

---

[warning] 723-723: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/audit/views.py` around lines 689 - 723, The view currently
trusts a client-supplied hq_url and uses server
COMMCARE_API_KEY/COMMCARE_USERNAME to fetch it; change it to accept a
server-side attachment identifier (e.g., attachment_id or blob_id) instead of
url, resolve the actual CommCare attachment path on the backend from the
audit/session/blob model, verify the current user's permission/domain matches
the attachment's allowed scope, then construct the trusted hq_url from the known
COMMCARE_HQ_URL plus the resolved path and call httpx.get (keeping existing
headers/timeout) — remove/disable using request.GET["url"] and the loose
host/path checks and ensure permission checks occur before any external fetch.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Filter by matching definition IDs, not just opportunity IDs.

When template_types is set, this only records which opportunities have at least one matching definition. The row builder still emits every run from those opportunities, so an opportunity that has both bulk_image_audit and some other workflow will leak unrelated runs into a filtered Audit of Audits report.

Possible fix

-        opps_with_matching_defs: set[int] = set()
+        opps_with_matching_defs: set[int] = set()
+        matching_definition_ids: set[int] = set()

         with ThreadPoolExecutor(max_workers=min(MAX_CONCURRENT_REQUESTS, len(opportunity_ids) or 1)) as ex:
             future_to_opp = {ex.submit(self._fetch_definitions_for_opp, oid): oid for oid in opportunity_ids}
             for future in as_completed(future_to_opp):
                 opp_id = future_to_opp[future]
                 for d in future.result():
                     def_map[d.id] = d
                     if self.template_types is None or d.template_type in self.template_types:
                         opps_with_matching_defs.add(opp_id)
+                        matching_definition_ids.add(d.id)

         rows = []
         for run in runs:
+            if self.template_types and run.definition_id not in matching_definition_ids:
+                continue
             definition = def_map.get(run.definition_id)

Also applies to: 285-286

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/custom_analysis/audit_of_audits/data_access.py` around lines
240 - 247, Currently the code only records opp IDs with any matching definitions
(opps_with_matching_defs) after calling self._fetch_definitions_for_opp, which
still allows unrelated runs from that opp to be emitted; change
opps_with_matching_defs from a set to a mapping of opp_id ->
set(matching_definition_ids) by collecting d.id for each definition d whose
d.template_type matches self.template_types when building def_map in the
ThreadPoolExecutor block (the block that calls self._fetch_definitions_for_opp
and updates def_map/d). Then update the row-building logic that emits runs to
consult that mapping and only emit runs whose run.definition_id is in the
matching_definition_ids for that opp (i.e., filter by definition IDs, not just
opportunity IDs). Apply the same change to the corresponding logic used in the
other occurrence mentioned (the later block that currently uses
opps_with_matching_defs).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Scope opportunity_ids to the selected organizations before building the report.

This request currently passes every opportunity from the user's session into AuditOfAuditsDataAccess, even when only one org is selected. That means the report fan-out does not actually narrow with the org filter and can still do unnecessary work against deselected organizations.

Also applies to: 192-197

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/custom_analysis/audit_of_audits/views.py` around lines 162 -
168, The current building of opportunity_ids and opp_name_map pulls every
opportunity from user_opps and sends them into AuditOfAuditsDataAccess even when
an org filter is selected; update the comprehensions that build opportunity_ids
and opp_name_map to first filter user_opps by the selected organization(s)
(e.g., check o.get("organization_id") or similar against the
selected_org_ids/selected_org variable in scope) so only opportunities belonging
to the selected org(s) are included, and make the same change in the other
occurrence around lines 192-197 where opportunities are assembled; ensure the
filtered opportunity_ids/opp_name_map are what's passed into
AuditOfAuditsDataAccess and any downstream report construction.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Finalize the run via the completion endpoint, not only onUpdateState.

This only patches run.data.state. Anything that reads the run's top-level status can keep treating the workflow as in_progress after this redirect. Use the run-completion action here, then persist the extra completion metadata in state.

Suggested fix

     const handleComplete = async () => {
         const avgPassed = computeAvgPassed(linkedSessions);
         setIsCompleting(true);
         setCompleteError(null);
         try {
+            await actions.completeRun({
+                overall_result: avgPassed !== null && avgPassed >= threshold ? 'pass' : 'fail',
+                notes: '',
+            });
             await onUpdateState({
                 phase: 'completed',
-                status: 'completed',
                 avg_passed: avgPassed,
                 completion: { completed_at: new Date().toISOString() },
             });
             const oppId = instance.opportunity_id || '';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/workflow/templates/bulk_image_audit.py` around lines 410 -
423, The handleComplete function currently only calls onUpdateState which
patches run.data.state; instead call the run-completion action (the API/method
that marks a run complete at top-level, e.g., invoke the run completion endpoint
or action named "run-completion" or completeRun) before or alongside persisting
the metadata, then call onUpdateState to save avg_passed and completion metadata
into run.data.state; update the try block in handleComplete to first await the
run-completion action (passing run id and completion payload), then await
onUpdateState with avg_passed and completion fields so both the run's top-level
status and the run.data.state are correctly persisted and the redirect will
reflect completed status.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use labs_context["opportunity_name"] when building run_data.

These branches populate run_data["opportunity_name"] from labs_context.get("opportunity", {}).get("name"), but this view already reads labs_context.get("opportunity_name") above. When the nested dict is absent, instance.opportunity_name is blank, which breaks the bulk image audit's default opportunity prefill on new, edit, and reopened runs.

Suggested fix

-                    "opportunity_name": labs_context.get("opportunity", {}).get("name"),
+                    "opportunity_name": labs_context.get("opportunity_name"),

Also applies to: 351-352, 377-378

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/workflow/views.py` around lines 334 - 335, Replace the
places that set run_data["opportunity_name"] from
labs_context.get("opportunity", {}).get("name") with the existing
labs_context["opportunity_name"] (or labs_context.get("opportunity_name")) so
run_data uses the same precomputed opportunity_name value the view reads
earlier; update the run_data assignments (the branches that build run_data) to
reference labs_context["opportunity_name"] instead of accessing the nested
"opportunity" dict (affecting the run_data construction around the symbols
run_data and labs_context and the usage that leads to
instance.opportunity_name).

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

commcare_connect/workflow/views.py (1)

362-391: ⚠️ Potential issue | 🔴 Critical

Critical bug: Run is created but then error is immediately returned.

Lines 369-375 create a new run, but lines 390-391 unconditionally return an error. The created run is orphaned and the user sees "Could not create a new run" despite a run being created in the database.

The comment on lines 387-389 states this branch shouldn't execute if opportunity_id exists (get() should redirect), but if that's true, the run creation code shouldn't be here at all.

Proposed fix: Remove the dead run-creation code from this error branch

             else:
-                # Create new run (always creates a fresh run)
-                from datetime import datetime, timedelta, timezone
-
-                today = datetime.now(timezone.utc).date()
-                week_start = today - timedelta(days=today.weekday())
-                week_end = week_start + timedelta(days=6)
-
-                run = data_access.create_run(
-                    definition_id=definition_id,
-                    opportunity_id=opportunity_id,
-                    period_start=week_start.isoformat(),
-                    period_end=week_end.isoformat(),
-                    initial_state={"worker_states": {}},
-                )
-                run_data = {
-                    "id": run.id,
-                    "definition_id": definition_id,
-                    "opportunity_id": opportunity_id,
-                    "opportunity_name": labs_context.get("opportunity", {}).get("name"),
-                    "status": run.data.get("status", "in_progress"),
-                    "state": run.data.get("state", {}),
-                    "period_start": run.data.get("period_start"),
-                    "period_end": run.data.get("period_end"),
-                }
-                context["is_edit_mode"] = False
                 # No run_id and not edit mode — get() should have redirected.
                 # This branch only executes if opportunity_id was missing at
                 # get() time (no labs context), so show a friendly error.
                 context["error"] = "Could not create a new run. Please select an opportunity."
                 return context

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/workflow/views.py` around lines 362 - 391, The branch that
sets context["error"] unconditionally is creating a run with
data_access.create_run (and building run_data) then immediately returning an
error; remove the run creation and run_data construction from this
error-handling branch so that create_run is only called where a valid
opportunity_id path exists, and keep this branch as a pure error path that sets
context["is_edit_mode"]=False and context["error"]="Could not create a new run.
Please select an opportunity." (leave references to create_run, run_data, and
context keys to locate the code to delete).

♻️ Duplicate comments (3)

commcare_connect/labs/analysis/backends/sql/backend.py (1)

116-121: ⚠️ Potential issue | 🟠 Major

Keep None and 0 distinct in raw-cache validation.

expected_visit_count or 0 makes an unknown count and a real zero-visit export equivalent. With the current SQLCacheManager.has_valid_raw_cache() implementation, passing 0 degrades to visit_count__gte=0, so any non-expired cache can satisfy these branches and return stale rows for opportunities that now have zero visits. Please preserve None as the “unknown” sentinel and handle that case separately instead of coercing it to 0.

Also applies to: 192-195

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/labs/analysis/backends/sql/backend.py` around lines 116 -
121, The code currently coerces expected_visit_count to 0 via
"expected_visit_count or 0", conflating a real zero-count with unknown; change
the logic to preserve None as the sentinel and only call
cache_manager.has_valid_raw_cache() with an integer when expected_visit_count is
not None (e.g. if expected_visit_count is None, call has_valid_raw_cache with a
parameter that signals “unknown” per SQLCacheManager API or call a different
overload/branch that omits visit_count), so update the branches around
force_refresh/expected_visit_count and the similar block at the other occurrence
(around lines 192-195) to treat None and 0 separately and pass the real value
(None vs int) into cache_manager.has_valid_raw_cache.

commcare_connect/workflow/views.py (1)

337-337: ⚠️ Potential issue | 🟠 Major

Use labs_context.get("opportunity_name") instead of nested dict access.

This line uses labs_context.get("opportunity", {}).get("name"), but opportunity_name is already extracted at line 256 using labs_context.get("opportunity_name"). The nested dict access may return None when the flat key would succeed.

Proposed fix

-                    "opportunity_name": labs_context.get("opportunity", {}).get("name"),
+                    "opportunity_name": labs_context.get("opportunity_name"),

Also applies to lines 354 and 380.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/workflow/views.py` at line 337, Replace the nested dict
access labs_context.get("opportunity", {}).get("name") with the flat key lookup
labs_context.get("opportunity_name") in the view where labs_context is used
(ensure you update all occurrences, including the instance at the current block
and the similar usages noted around the other locations). Locate references to
labs_context and change those three spots so they consistently call
labs_context.get("opportunity_name") instead of drilling into the "opportunity"
dict.

commcare_connect/templates/workflow/list.html (1)

873-913: ⚠️ Potential issue | 🟠 Major

Use one threshold key here and stop hiding valid 0 metrics.

Line 875 still colors against run.state.threshold while the Threshold column reads run.state.pass_threshold, so non-default runs can be shown in the wrong color. The truthy checks on Lines 886, 893, 900, and 907 also turn legitimate 0 values into —.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/templates/workflow/list.html` around lines 873 - 913, The
template uses run.state.threshold for coloring but displays
run.state.pass_threshold and also hides legitimate zero values via truthy
checks; inside the {% with %} change threshold=run.state.threshold|default:80 to
threshold=run.state.pass_threshold|default:80, and replace the truthy
conditionals for pass_threshold, sample_percentage, images_reviewed, and
tasks_created (the {% if run.state.pass_threshold %}, {% if
run.state.sample_percentage %}, {% if run.state.images_reviewed %}, {% if
run.state.tasks_created %}) with null-checks that accept 0 (e.g., {% if
run.state.pass_threshold is not none %} or {% if run.state.X is not none and
run.state.X != "" %}) so zeros render instead of being shown as “—”.

🧹 Nitpick comments (1)

commcare_connect/workflow/templates/__init__.py (1)

220-239: Keep __all__ aligned with the explicit re-export block.

bulk_image_audit is now part of the public surface, but it still is not imported in the from . import (...) block above. That makes the backwards-compatibility section depend on template-discovery side effects instead of the explicit re-export path used for the other templates.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/workflow/templates/__init__.py` around lines 220 - 239, The
__all__ export list includes "bulk_image_audit" but the explicit re-export
import block (the from . import (...) tuple) does not import bulk_image_audit;
add bulk_image_audit to that import tuple (alongside audit_with_ai_review,
kmc_flw_flags, etc.) so the explicit re-export path and the __all__ list stay
aligned and the module is properly exported via the package's __init__.py.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@commcare_connect/templates/workflow/list.html`:
- Around line 51-58: The template hides the button but you must enforce the same
Dimagi-only rule inside create_workflow_from_template_view; add a server-side
check at the top of the view (or a decorator) that verifies the caller is in the
Dimagi allowlist/is_dimagi condition and return an HTTP 403 (or raise
PermissionDenied) for non-allowlisted users before processing POST data; update
create_workflow_from_template_view to perform this check (and any helper it
calls) so direct POSTs cannot create workflows from templates.

---

Outside diff comments:
In `@commcare_connect/workflow/views.py`:
- Around line 362-391: The branch that sets context["error"] unconditionally is
creating a run with data_access.create_run (and building run_data) then
immediately returning an error; remove the run creation and run_data
construction from this error-handling branch so that create_run is only called
where a valid opportunity_id path exists, and keep this branch as a pure error
path that sets context["is_edit_mode"]=False and context["error"]="Could not
create a new run. Please select an opportunity." (leave references to
create_run, run_data, and context keys to locate the code to delete).

---

Duplicate comments:
In `@commcare_connect/labs/analysis/backends/sql/backend.py`:
- Around line 116-121: The code currently coerces expected_visit_count to 0 via
"expected_visit_count or 0", conflating a real zero-count with unknown; change
the logic to preserve None as the sentinel and only call
cache_manager.has_valid_raw_cache() with an integer when expected_visit_count is
not None (e.g. if expected_visit_count is None, call has_valid_raw_cache with a
parameter that signals “unknown” per SQLCacheManager API or call a different
overload/branch that omits visit_count), so update the branches around
force_refresh/expected_visit_count and the similar block at the other occurrence
(around lines 192-195) to treat None and 0 separately and pass the real value
(None vs int) into cache_manager.has_valid_raw_cache.

In `@commcare_connect/templates/workflow/list.html`:
- Around line 873-913: The template uses run.state.threshold for coloring but
displays run.state.pass_threshold and also hides legitimate zero values via
truthy checks; inside the {% with %} change
threshold=run.state.threshold|default:80 to
threshold=run.state.pass_threshold|default:80, and replace the truthy
conditionals for pass_threshold, sample_percentage, images_reviewed, and
tasks_created (the {% if run.state.pass_threshold %}, {% if
run.state.sample_percentage %}, {% if run.state.images_reviewed %}, {% if
run.state.tasks_created %}) with null-checks that accept 0 (e.g., {% if
run.state.pass_threshold is not none %} or {% if run.state.X is not none and
run.state.X != "" %}) so zeros render instead of being shown as “—”.

In `@commcare_connect/workflow/views.py`:
- Line 337: Replace the nested dict access labs_context.get("opportunity",
{}).get("name") with the flat key lookup labs_context.get("opportunity_name") in
the view where labs_context is used (ensure you update all occurrences,
including the instance at the current block and the similar usages noted around
the other locations). Locate references to labs_context and change those three
spots so they consistently call labs_context.get("opportunity_name") instead of
drilling into the "opportunity" dict.

---

Nitpick comments:
In `@commcare_connect/workflow/templates/__init__.py`:
- Around line 220-239: The __all__ export list includes "bulk_image_audit" but
the explicit re-export import block (the from . import (...) tuple) does not
import bulk_image_audit; add bulk_image_audit to that import tuple (alongside
audit_with_ai_review, kmc_flw_flags, etc.) so the explicit re-export path and
the __all__ list stay aligned and the module is properly exported via the
package's __init__.py.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 60c2050a-eb39-4897-b64b-ab0f03115976

📥 Commits

Reviewing files that changed from the base of the PR and between 5abb95f and 76f7e87.

📒 Files selected for processing (5)

• commcare_connect/labs/analysis/backends/sql/backend.py
• commcare_connect/templates/workflow/list.html
• commcare_connect/workflow/data_access.py
• commcare_connect/workflow/templates/__init__.py
• commcare_connect/workflow/views.py

🚧 Files skipped from review as they are similar to previous changes (1)

• commcare_connect/workflow/data_access.py

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

commcare_connect/tasks/views.py (1)

470-474: ⚠️ Potential issue | 🟡 Minor

Resource leak: data_access may not be closed on unexpected exceptions.

If an exception is raised after data_access is initialized (line 441) but before the return at line 468, the close() call at line 466 is skipped. The new catch-all exception handler at lines 472-474 doesn't close the data access client.

🛠️ Suggested fix using try/finally

 def task_bulk_create(request):
     """Create multiple tasks at once (bulk creation)."""
+    data_access = None
     try:
         body = json.loads(request.body)
         opportunity_id = body.get("opportunity_id")
         ...
         data_access = TaskDataAccess(user=request.user, request=request)
         ...
-        data_access.close()
-
         return JsonResponse({"success": True, "created_count": created_count, "tasks": created_tasks, "errors": errors})

     except json.JSONDecodeError:
         return JsonResponse({"success": False, "error": "Invalid JSON"}, status=400)
     except Exception as e:
         logger.error(f"Error in bulk task creation: {e}", exc_info=True)
         return JsonResponse({"success": False, "error": str(e)}, status=500)
+    finally:
+        if data_access:
+            data_access.close()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/tasks/views.py` around lines 470 - 474, The exception
handler currently logs errors but can leak the data_access client if an
exception occurs after it is initialized; modify the function to guarantee
data_access.close() is always called by wrapping the block that creates/uses
data_access in a try/finally (or use a context manager) so that
data_access.close() runs in the finally regardless of exceptions, and only call
close() if data_access was successfully created (e.g., check for None). Ensure
the catch-all except Exception as e still logs/returns as before but the finally
performs the cleanup of data_access.

♻️ Duplicate comments (2)

commcare_connect/workflow/views.py (1)

337-337: ⚠️ Potential issue | 🟠 Major

Use labs_context.get("opportunity_name") instead of nested dict access.

This line still uses labs_context.get("opportunity", {}).get("name") to populate opportunity_name, but the view already reads labs_context.get("opportunity_name") at line 256. When the nested dict is absent, run_data["opportunity_name"] will be None, breaking the opportunity name prefill.

🛠️ Suggested fix

                 run_data = {
                     "id": 0,  # Temporary ID
                     "definition_id": definition_id,
                     "opportunity_id": opportunity_id,
-                    "opportunity_name": labs_context.get("opportunity", {}).get("name"),
+                    "opportunity_name": labs_context.get("opportunity_name"),
                     "status": "preview",

Also apply the same fix at lines 354 (existing run branch).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/workflow/views.py` at line 337, The code sets
run_data["opportunity_name"] using labs_context.get("opportunity",
{}).get("name") which can be None if the nested dict is missing; change it to
use labs_context.get("opportunity_name") instead so it reads the same flattened
key as elsewhere. Update the assignment that builds run_data (the dict where
"opportunity_name" is being set) to use labs_context.get("opportunity_name"),
and apply the identical change in the other branch that builds run_data for the
existing-run path (the block that currently uses labs_context.get("opportunity",
{}).get("name")) to keep both branches consistent.

commcare_connect/audit/hq_app_utils.py (1)

62-70: ⚠️ Potential issue | 🟡 Minor

Edge case: ID collision remains possible when xmlns is empty.

When form_xmlns is empty and both leaf_id and image_path are already in seen_ids, line 69 falls back to image_path without the xmlns qualifier. This results in a duplicate ID being added to seen_ids and potentially to the results.

🛠️ Suggested fix

                 # Use leaf ID when unique; fall back to full path, then xmlns-qualified path
                 leaf_id = _question_id(xform_path)
                 if leaf_id not in seen_ids:
                     question_id = leaf_id
                 elif image_path not in seen_ids:
                     question_id = image_path
                 else:
-                    question_id = f"{form_xmlns}:{image_path}" if form_xmlns else image_path
+                    # Use xmlns if available; otherwise use form_name as discriminator
+                    discriminator = form_xmlns or form_name or str(len(seen_ids))
+                    question_id = f"{discriminator}:{image_path}"
                 seen_ids.add(question_id)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/audit/hq_app_utils.py` around lines 62 - 70, The fallback
branch can still produce duplicates when form_xmlns is empty; modify the else
branch that sets question_id (where leaf_id, image_path, form_xmlns and seen_ids
are used) so it produces a distinct identifier when form_xmlns is falsy — e.g.
use a sentinel prefix or other stable qualifier instead of plain image_path (for
example question_id = f"<no-xmlns>:{image_path}" when not form_xmlns) before
adding to seen_ids to guarantee uniqueness.

🧹 Nitpick comments (4)

commcare_connect/audit/views.py (2)

1839-1896: New image questions endpoint looks good with proper auth and validation.

The endpoint correctly:

1. Requires OAuth token
2. Validates cc_domain and cc_app_id presence
3. Uses CommCare API key auth for HQ requests

Two minor improvements suggested by static analysis:

♻️ Use logging.exception for better stack traces

         except Exception as e:
-            logger.error(f"[ImageQuestions] Failed to fetch opportunity {opp_id} metadata: {e}")
+            logger.exception(f"[ImageQuestions] Failed to fetch opportunity {opp_id} metadata: {e}")
             return JsonResponse({"error": "Failed to fetch opportunity metadata"}, status=502)

         except Exception as e:
-            logger.error(f"[ImageQuestions] Failed to fetch HQ app {cc_app_id}: {e}")
+            logger.exception(f"[ImageQuestions] Failed to fetch HQ app {cc_app_id}: {e}")
             return JsonResponse({"error": "Failed to fetch app definition from CommCare HQ"}, status=502)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/audit/views.py` around lines 1839 - 1896, Summary: Use
logger.exception to capture stack traces in the exception handlers inside
OpportunityImageQuestionsAPIView.get. Fix: In the except blocks that currently
call logger.error in OpportunityImageQuestionsAPIView.get (the
fetch_opportunity_metadata exception handler and the httpx/app fetch exception
handler that log "[ImageQuestions] Failed to fetch opportunity ..." and
"[ImageQuestions] Failed to fetch HQ app ..."), replace logger.error(...) with
logger.exception(...) while keeping the same message text so the stack trace is
recorded; leave the existing JsonResponse behavior unchanged.

---

431-436: Silently swallowing exceptions hides debugging information.

The bare except Exception: pass makes it impossible to diagnose FLW name fetch failures. Consider logging at debug level so issues can be traced when needed.

♻️ Proposed fix

             # Fetch FLW display names for the opportunity
             flw_names = {}
             try:
                 flw_names = data_access.get_flw_names(opportunity_id)
-            except Exception:
-                pass
+            except Exception as e:
+                logger.debug("Failed to fetch FLW names for opp %s: %s", opportunity_id, e)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/audit/views.py` around lines 431 - 436, The try/except
around the FLW name fetch swallows all errors; update the block that calls
data_access.get_flw_names(opportunity_id) so it still recovers on failure but
logs the exception at debug (or appropriate) level instead of silently passing;
reference flw_names, get_flw_names and opportunity_id and use the module/logger
already in scope (e.g., logger.debug or processLogger) to include the exception
message and context before falling back to the empty flw_names.

commcare_connect/templates/audit/bulk_assessment.html (1)

1097-1108: Consider handling workflow state update failure more visibly.

The empty catch block on line 1107 silently ignores workflow state persistence failures. While marked as "non-critical", a failed state update could cause task button state to be lost on page reload.

♻️ Optional: Log warning for debugging

-              } catch (_) { /* non-critical, ignore */ }
+              } catch (e) { console.warn('Failed to persist task state to workflow:', e); }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/templates/audit/bulk_assessment.html` around lines 1097 -
1108, The empty catch swallowing failures from the workflow state POST (the
fetch call when workflowRunId is present) should at least log the error so
state-persistence failures are visible; update the catch block for the fetch in
the code that references workflowRunId, this.csrfToken, this.createdTaskFLWs and
this.flwTasks to log a warning (including the caught error and workflowRunId)
via your client logger or console.warn, and consider adding a benign
user-visible notice (e.g., non-blocking toast) so that task button state loss
can be diagnosed on reload.

commcare_connect/templates/custom_analysis/audit_of_audits/report.html (1)

105-112: Duplicate options handled by client-side deduplication.

The template loop creates duplicate <option> elements when multiple runs share the same opportunity. This is correctly addressed by dedupOpportunitySelect() on DOMContentLoaded (line 531). For large datasets, consider server-side deduplication to reduce initial HTML size.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@commcare_connect/templates/custom_analysis/audit_of_audits/report.html`
around lines 105 - 112, The template currently renders an <option> for every row
with an opportunity_id, causing duplicate options; instead perform server-side
deduplication in the template by tracking seen opportunity_id values and only
emitting the first option for each unique id (preserve use of opportunity_name
when present). Update the loop that iterates over rows (the block referencing
row.opportunity_id and row.opportunity_name) to skip rendering if the
opportunity_id has already been output (e.g., maintain a seen set/list in the
template and append each id when you render its option). This reduces HTML size
for large datasets and complements the existing dedupOpportunitySelect()
client-side function.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@commcare_connect/tasks/views.py`:
- Around line 470-474: The exception handler currently logs errors but can leak
the data_access client if an exception occurs after it is initialized; modify
the function to guarantee data_access.close() is always called by wrapping the
block that creates/uses data_access in a try/finally (or use a context manager)
so that data_access.close() runs in the finally regardless of exceptions, and
only call close() if data_access was successfully created (e.g., check for
None). Ensure the catch-all except Exception as e still logs/returns as before
but the finally performs the cleanup of data_access.

---

Duplicate comments:
In `@commcare_connect/audit/hq_app_utils.py`:
- Around line 62-70: The fallback branch can still produce duplicates when
form_xmlns is empty; modify the else branch that sets question_id (where
leaf_id, image_path, form_xmlns and seen_ids are used) so it produces a distinct
identifier when form_xmlns is falsy — e.g. use a sentinel prefix or other stable
qualifier instead of plain image_path (for example question_id =
f"<no-xmlns>:{image_path}" when not form_xmlns) before adding to seen_ids to
guarantee uniqueness.

In `@commcare_connect/workflow/views.py`:
- Line 337: The code sets run_data["opportunity_name"] using
labs_context.get("opportunity", {}).get("name") which can be None if the nested
dict is missing; change it to use labs_context.get("opportunity_name") instead
so it reads the same flattened key as elsewhere. Update the assignment that
builds run_data (the dict where "opportunity_name" is being set) to use
labs_context.get("opportunity_name"), and apply the identical change in the
other branch that builds run_data for the existing-run path (the block that
currently uses labs_context.get("opportunity", {}).get("name")) to keep both
branches consistent.

---

Nitpick comments:
In `@commcare_connect/audit/views.py`:
- Around line 1839-1896: Summary: Use logger.exception to capture stack traces
in the exception handlers inside OpportunityImageQuestionsAPIView.get. Fix: In
the except blocks that currently call logger.error in
OpportunityImageQuestionsAPIView.get (the fetch_opportunity_metadata exception
handler and the httpx/app fetch exception handler that log "[ImageQuestions]
Failed to fetch opportunity ..." and "[ImageQuestions] Failed to fetch HQ app
..."), replace logger.error(...) with logger.exception(...) while keeping the
same message text so the stack trace is recorded; leave the existing
JsonResponse behavior unchanged.
- Around line 431-436: The try/except around the FLW name fetch swallows all
errors; update the block that calls data_access.get_flw_names(opportunity_id) so
it still recovers on failure but logs the exception at debug (or appropriate)
level instead of silently passing; reference flw_names, get_flw_names and
opportunity_id and use the module/logger already in scope (e.g., logger.debug or
processLogger) to include the exception message and context before falling back
to the empty flw_names.

In `@commcare_connect/templates/audit/bulk_assessment.html`:
- Around line 1097-1108: The empty catch swallowing failures from the workflow
state POST (the fetch call when workflowRunId is present) should at least log
the error so state-persistence failures are visible; update the catch block for
the fetch in the code that references workflowRunId, this.csrfToken,
this.createdTaskFLWs and this.flwTasks to log a warning (including the caught
error and workflowRunId) via your client logger or console.warn, and consider
adding a benign user-visible notice (e.g., non-blocking toast) so that task
button state loss can be diagnosed on reload.

In `@commcare_connect/templates/custom_analysis/audit_of_audits/report.html`:
- Around line 105-112: The template currently renders an <option> for every row
with an opportunity_id, causing duplicate options; instead perform server-side
deduplication in the template by tracking seen opportunity_id values and only
emitting the first option for each unique id (preserve use of opportunity_name
when present). Update the loop that iterates over rows (the block referencing
row.opportunity_id and row.opportunity_name) to skip rendering if the
opportunity_id has already been output (e.g., maintain a seen set/list in the
template and append each id when you render its option). This reduces HTML size
for large datasets and complements the existing dedupOpportunitySelect()
client-side function.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 032932aa-86a9-4e5b-962b-e14753f3678b

📥 Commits

Reviewing files that changed from the base of the PR and between 76f7e87 and b610a57.

📒 Files selected for processing (10)

• commcare_connect/audit/data_access.py
• commcare_connect/audit/hq_app_utils.py
• commcare_connect/audit/tests/test_hq_app_utils.py
• commcare_connect/audit/views.py
• commcare_connect/custom_analysis/audit_of_audits/data_access.py
• commcare_connect/tasks/views.py
• commcare_connect/templates/audit/bulk_assessment.html
• commcare_connect/templates/custom_analysis/audit_of_audits/report.html
• commcare_connect/templates/workflow/list.html
• commcare_connect/workflow/views.py

🚧 Files skipped from review as they are similar to previous changes (2)

• commcare_connect/templates/workflow/list.html
• commcare_connect/audit/tests/test_hq_app_utils.py