fixes to reviewer UI

.env.development.example

@@ -23,3 +23,9 @@ EXTERNAL_API_KEY=dev_api_key
 # SLACK_CONTRIBUTORS_CHANNEL_ID=
 # Optional: Slack channel for streak break + leaderboard messages (#forge)
 # SLACK_STREAKS_CHANNEL_ID=
+
+# Read-only Airtable PAT for the YSWS Unified Database, used by the review
+# duplicate/double-dip scan. Needs data.records:read on the unified base.
+# Production sets this through Admin -> API keys instead, which stores it
+# encrypted in the database and survives deploys.
+UNIFIED_DB_PAT=

app/controllers/admin/projects_controller.rb

@@ -1,9 +1,9 @@
 class Admin::ProjectsController < Admin::ApplicationController
-  REVIEW_SCREEN_ACTIONS = %i[review repo_tree commit_stats changes_since_review ai_requirements_check ai_requirements_check_status].freeze
+  REVIEW_SCREEN_ACTIONS = %i[review repo_tree commit_stats changes_since_review ai_requirements_check ai_requirements_check_status check_draft_justification].freeze
 
   before_action :require_projects_permission!, except: REVIEW_SCREEN_ACTIONS + [ :unflag_for_review ]
   before_action :require_review_screen_access!, only: REVIEW_SCREEN_ACTIONS
-  before_action :set_project, only: [ :show, :review, :destroy, :restore, :toggle_hidden, :toggle_shadow_ban, :toggle_staff_pick, :change_tier, :add_note, :destroy_note, :update_note, :flag_for_review, :unflag_for_review, :mark_unbuilt, :reverse_review, :ai_requirements_check, :ai_requirements_check_status, :repo_tree, :commit_stats, :changes_since_review, :send_checkpoint_message, :send_dm_message ]
+  before_action :set_project, only: [ :show, :review, :destroy, :restore, :toggle_hidden, :toggle_shadow_ban, :toggle_staff_pick, :change_tier, :add_note, :destroy_note, :update_note, :flag_for_review, :unflag_for_review, :mark_unbuilt, :reverse_review, :ai_requirements_check, :ai_requirements_check_status, :check_draft_justification, :repo_tree, :commit_stats, :changes_since_review, :send_checkpoint_message, :send_dm_message ]
 
   def index
     scope = policy_scope(Project).includes(:user, :ships)
@@ -360,7 +360,16 @@ def unflag_for_review
   # reviewing itself". JustificationLint already runs on every keystroke for
   # free; this is the paid second opinion, so it is on demand only.
   def check_draft_justification
-    authorize @project, :review?
+    # Deliberately not `authorize`: Pundit's failure handler redirects to an HTML
+    # page, and a fetch() follows that redirect and then chokes parsing HTML as
+    # JSON — which surfaces to the reviewer as a generic "request failed" with no
+    # hint that it was a permissions problem.
+    unless policy(@project).review?
+      return render json: {
+        result: { "overall" => "error",
+                  "message" => "You don't have review permission for #{@project.review_tier.to_s.tr('_', ' ')}, so the audit can't run." }
+      }, status: :forbidden
+    end
 
     result = AiRequirementsChecker.audit_justification(
       text: params[:justification].to_s,
@@ -371,6 +380,13 @@ def check_draft_justification
     render json: { result: result }
   rescue AiRequirementsChecker::Error => e
     render json: { result: { "overall" => "error", "message" => e.message } }
+  rescue StandardError => e
+    # A timeout or a malformed model response should say so, not 500 into the
+    # frontend's catch-all.
+    Rails.logger.error("[JustificationAudit] project=#{@project&.id.inspect} #{e.class}: #{e.message}")
+    Sentry.capture_exception(e) if defined?(Sentry)
+    render json: { result: { "overall" => "error", "message" => "The audit hit an unexpected error (#{e.class}). Try again." } },
+           status: :internal_server_error
   end
 
   REQUIREMENTS_CHECKER_DECISIONS = %w[requirements_met return].freeze

app/javascript/components/admin/review/types.ts

@@ -167,6 +167,7 @@ export interface DuplicateScan {
   macondo: { id: string; title: string | null; shipped: boolean }[]
   unified_available: boolean
   unified_error: boolean
+  unified_checked: boolean
   verdict: 'clear' | 'review' | 'blocked'
   reason?: string
   scanned_at: string

app/javascript/lib/justificationPreview.ts

@@ -111,7 +111,8 @@ export function buildJustification(ctx: JustificationContext): string {
   const deflationRaw = Math.max(0, Math.round((ctx.claimed_hours - ctx.approved_hours) * 10) / 10)
   const deflation = String(deflationRaw).replace(/\.0$/, '')
   const deflationReason = (ctx.deflation_reason ?? '').trim()
-  const deflationSuffix = deflationRaw > 0 && deflationReason ? ` — reason: ${deflationReason}` : ''
+  // A multi-line list of per-entry reasons reads as a block, not a clause.
+  const deflationSuffix = deflationRaw > 0 && deflationReason ? `:\n${deflationReason}` : ''
   const reasoning = (ctx.assessment ?? '').trim() || '(no justification provided)'
   const additional = (ctx.additional_justification ?? '').trim()
   const additionalBlock = additional ? `\nAdditional justification:\n${additional}\n` : ''

app/javascript/pages/Admin/AirtableQueue/Show.tsx

@@ -293,7 +293,7 @@ export default function AdminAirtableQueueShow({
         <CardContent className="space-y-3">
           <div className="flex flex-col md:flex-row gap-2 md:items-center justify-between">
             <p className="text-sm text-muted-foreground">
-              Verifies if the justification is up to standard using gemini :D
+              Audits the justification against the Unified DB standard before it goes out.
             </p>
             <Button variant="outline" size="sm" onClick={runCheck} disabled={checking}>
               {checking ? <Loader2 className="size-4 animate-spin" /> : <Sparkles className="size-4" />}

app/javascript/pages/Admin/Reviews/Show.tsx

@@ -345,9 +345,11 @@ export default function AdminReviewsShow({
         .filter((e) => approvedFor(e, deflations[e.id]) < claimedFor(e) - 0.001)
         .map((e) => {
           const reason = (deflations[e.id]?.reason ?? '').trim()
-          return `"${e.title}" ${claimedFor(e).toFixed(1)}h → ${approvedFor(e, deflations[e.id]).toFixed(1)}h: ${reason}`
+          // Entry titles contain colons ("August 12: Competition date!"), so the
+          // reason is separated with an em dash to stay unambiguous.
+          return `- "${e.title}" ${claimedFor(e).toFixed(1)}h → ${approvedFor(e, deflations[e.id]).toFixed(1)}h — ${reason}`
         })
-        .join(' '),
+        .join('\n'),
     [project.devlogs, deflations],
   )
 
@@ -524,10 +526,30 @@ export default function AdminReviewsShow({
         },
         body: JSON.stringify({ justification: justificationPreview, approved_hours: approvedHours }),
       })
-      const data = res.ok ? await res.json() : null
-      setJustificationAudit(data?.result ?? { overall: 'error', message: 'The audit request failed.' })
-    } catch {
-      setJustificationAudit({ overall: 'error', message: 'The audit request failed.' })
+      // The endpoint answers with a JSON result even on failure, so read the
+      // body regardless of status. Anything that is not JSON (an auth redirect
+      // landing on an HTML page, a proxy error page) is reported with its status
+      // rather than collapsed into one unhelpful message.
+      const body = await res.text()
+      let parsed: { result?: AiCheckResult } | null = null
+      try {
+        parsed = JSON.parse(body)
+      } catch {
+        parsed = null
+      }
+      setJustificationAudit(
+        parsed?.result ?? {
+          overall: 'error',
+          message: res.redirected
+            ? `The audit request was redirected to ${res.url} — you may not have review permission for this tier.`
+            : `The audit request failed (HTTP ${res.status}).`,
+        },
+      )
+    } catch (e) {
+      setJustificationAudit({
+        overall: 'error',
+        message: `Could not reach the audit endpoint: ${e instanceof Error ? e.message : 'network error'}`,
+      })
     } finally {
       setJustificationAuditing(false)
     }

app/lib/justification_template.rb

@@ -54,7 +54,9 @@ def self.render_text(iteration:, project:, ship_type:, ship_name:, submittion_ti
     deflation = (claimed_hours.to_f - approved_hours.to_f).round(1)
     deflation = 0 if deflation.negative?
     deflation_reason = fields[:deflation_reason].to_s.strip
-    deflation_suffix = deflation > 0 && deflation_reason.present? ? " — reason: #{deflation_reason}" : ""
+    # Per-entry reasons arrive newline-separated, so they render as a list under
+    # the total rather than one unreadable run-on line.
+    deflation_suffix = deflation > 0 && deflation_reason.present? ? ":\n#{deflation_reason}" : ""
 
     format(
       TEMPLATE,

app/services/duplicate_scan.rb

@@ -24,10 +24,12 @@ def run(project, refresh: false)
     return cached if cached.present?
 
     result = scan(project, slug)
-    # A failed cross-program lookup must not be remembered as "clear" — that
-    # would hide a real double-dip for the whole TTL. Leave it uncached so the
-    # next page load retries.
-    Rails.cache.write(key, result, expires_in: CACHE_TTL) unless result["unified_error"]
+    # Only remember a scan that actually checked every program. A failed lookup
+    # must not be cached as "clear" (it would hide a real double-dip for the
+    # whole TTL), and neither must a scan taken before the Unified DB token was
+    # configured — otherwise setting the token appears to do nothing until the
+    # cache expires.
+    Rails.cache.write(key, result, expires_in: CACHE_TTL) if result["unified_checked"]
     result
   end
 
@@ -51,6 +53,7 @@ def scan(project, slug)
       "macondo" => macondo,
       "unified_available" => UnifiedDbService.enabled?,
       "unified_error" => unified_error,
+      "unified_checked" => UnifiedDbService.enabled? && !unified_error,
       "verdict" => verdict(forge, unified, macondo),
       "scanned_at" => Time.current.iso8601
     }
@@ -108,6 +111,7 @@ def verdict(forge, unified, macondo)
   def empty_result(reason)
     { "slug" => nil, "forge" => [], "unified" => [], "macondo" => [],
       "unified_available" => UnifiedDbService.enabled?, "unified_error" => false,
-      "verdict" => "clear", "reason" => reason, "scanned_at" => Time.current.iso8601 }
+      "unified_checked" => false, "verdict" => "clear", "reason" => reason,
+      "scanned_at" => Time.current.iso8601 }
   end
 end

test/controllers/admin/reviews_fine_guard_test.rb

@@ -274,6 +274,60 @@ def deflations(rows)
     assert_equal "approved", @project.reload.status
   end
 
+  # --- justification audit endpoint ---------------------------------------
+  #
+  # A fetch() follows redirects, so answering a JSON request with Pundit's HTML
+  # redirect made every permission failure look like a generic network error.
+
+  test "the audit endpoint reports a permission problem as JSON, not a redirect" do
+    @reviewer.update!(permissions: %w[projects pending_reviews])
+
+    post check_draft_justification_admin_project_path(@project), params: { justification: "text", approved_hours: 12.4 }
+
+    assert_response :forbidden
+    body = JSON.parse(response.body)
+    assert_equal "error", body.dig("result", "overall")
+    assert_match(/review permission/i, body.dig("result", "message"))
+  end
+
+  test "a requirements checker gets a readable message, not a bare 404" do
+    checker = make_user(roles: %w[user reviewer], permissions: %w[review_requirements], birthday: Date.new(2008, 1, 1))
+    sign_in_as(checker)
+
+    post check_draft_justification_admin_project_path(@project), params: { justification: "text", approved_hours: 12.4 }
+
+    assert_response :forbidden
+    assert_match(/review permission/i, JSON.parse(response.body).dig("result", "message"))
+  end
+
+  test "the audit endpoint reports a missing AI credential as JSON" do
+    post check_draft_justification_admin_project_path(@project), params: { justification: "text", approved_hours: 12.4 }
+
+    assert_response :success
+    body = JSON.parse(response.body)
+    assert_equal "error", body.dig("result", "overall")
+    assert_match(/credential|configured/i, body.dig("result", "message"))
+  end
+
+  # The per-entry reasons used to be joined with a space, producing one
+  # unreadable run-on line in the Unified DB record.
+  test "deflation reasons render as a list, one entry per line" do
+    entry = @project.devlogs.first
+    approve(
+      override_hours: "6.0",
+      override_hours_justification: %(- "#{entry.title}" 12.4h → 6.0h — Entry describes writing up earlier work.),
+      **deflations([ {
+        devlog_id: entry.id, approved_hours: 6.0,
+        reason: "Entry describes writing up earlier work."
+      } ])
+    )
+
+    assert_equal "approved", @project.reload.status
+    justification = @project.approval_justification
+    assert_match(/hours of deflation was applied to meet our requirements:\n- "/, justification)
+    refute_match(/requirements — reason:/, justification)
+  end
+
   test "still enforces the checklist" do
     approve(checklist: [])
     assert_equal "pending", @project.reload.status

test/services/duplicate_scan_test.rb

@@ -148,6 +148,33 @@ def with_unified(records_for_repo:, enabled: true)
     assert_equal 1, calls, "expected the second call to be served from cache"
   end
 
+  # Setting the token has to take effect on the next page load, not 30 minutes
+  # later. A scan taken while it was unset used to be cached as a clean result.
+  test "a scan taken before the token was configured is not cached" do
+    calls = 0
+    counting = ->(*) { calls += 1; [] }
+
+    with_memory_cache do
+      with_unified(records_for_repo: counting, enabled: false) do
+        first = DuplicateScan.run(@project)
+        assert_not first["unified_checked"]
+        assert_not first["unified_available"]
+      end
+
+      # Token now configured; the next load must re-scan rather than serve the
+      # "not configured" answer from cache.
+      with_unified(records_for_repo: counting) do
+        second = DuplicateScan.run(@project)
+        assert second["unified_checked"], "expected a fresh scan once the token exists"
+      end
+    end
+
+    # Two calls, not one: the second load re-scanned instead of returning the
+    # cached "not configured" result. (The stub bypasses the real enabled?
+    # short-circuit, so the disabled pass counts as a call too.)
+    assert_equal 2, calls, "expected the second load to re-scan, not hit cache"
+  end
+
   test "a unified match blocks regardless of what Forge holds" do
     match = ->(*) {
       [ { "record_id" => "rec1", "record_url" => "https://airtable.com/x", "program" => "Blueprint",