refactor: remove duplicated review target rendering and validation

The review question sentence was written in four places and the URL
safety rules in three. Collapse each to one definition.

- Add `ReviewTarget::question_text_with_link` as the single definition of
  the question wording. `question_text()` and the Slack header both use
  it, so a wording change is now one edit.
- Delete `ReviewTargetKind::noun()`. The enum already derives
  `strum::Display` with the same snake_case output.
- Share one `review_target_line` helper between the console interviewer
  and the CLI attach client, which held a byte-identical copy. Print only
  the URL: `question.text` already carries the label and the noun.
- Trim the web-side check to the URL scheme, host, and credentials, which
  are what a raw `href` can act on. Label length and control characters
  cannot affect the DOM and stay server-side.
- Split validation from presentation in the web UI. `safeReviewTarget`
  returns the target or null, and each caller picks its own fallback, so
  an unsafe target now falls back to the same Markdown rendering as a
  question with no target.
- Derive the resource noun from `kind` in the web UI instead of
  hardcoding "document".
- Use `ReviewTargetKind.DOCUMENT` and the shared `isRecord` guard when
  parsing events, instead of a raw string and a hand-rolled object check
  that accepted arrays.
- Drop `deny_unknown_fields` from the wire struct. The OpenAPI schema
  leaves `additionalProperties` permissive, so an added field would
  otherwise make persisted events unreadable.
- Import `ReviewTarget` by name, and stop naming Slack in a fabro-types
  error message.
- Document that `review_target=true` replaces the gate's `label`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

apps/fabro-web/app/components/interview-dock.tsx

@@ -22,9 +22,14 @@ import {
 } from "../lib/mutations";
 import { ApiError } from "../lib/api-client";
 import { displayLabel } from "./interview-label";
-import { ReviewTargetQuestion } from "./review-target-question";
+import {
+  ReviewTargetQuestion,
+  safeReviewTarget,
+} from "./review-target-question";
 import { ErrorMessage } from "./ui";
 
+const QUESTION_TEXT = "text-pretty text-base/6 font-medium text-fg";
+
 const PRIMARY_BUTTON =
   "inline-flex items-center justify-center gap-1.5 rounded-lg bg-teal-500 px-3.5 py-2 text-sm font-medium text-on-primary transition-colors hover:bg-teal-300 focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-teal-500 disabled:cursor-not-allowed disabled:opacity-60 disabled:hover:bg-teal-500";
 
@@ -78,6 +83,7 @@ function InterviewQuestionDock({
   const submitMutation = useSubmitInterviewAnswer(runId);
   const [error, setError] = useState<string | null>(null);
   const submitting = submitMutation.isMutating;
+  const reviewTarget = safeReviewTarget(question.review_target);
 
   const submit = useCallback(
     async (answer: SubmitInterviewAnswer) => {
@@ -100,11 +106,14 @@ function InterviewQuestionDock({
       />
       <div className="space-y-5 px-5 py-4 sm:px-6">
         <div>
-          <ReviewTargetQuestion
-            reviewTarget={question.review_target}
-            fallbackText={question.text}
-            className="text-pretty text-base/6 font-medium text-fg"
-          />
+          {reviewTarget ? (
+            <ReviewTargetQuestion
+              target={reviewTarget}
+              className={QUESTION_TEXT}
+            />
+          ) : (
+            <p className={QUESTION_TEXT}>{question.text}</p>
+          )}
           <p className="mt-1 text-xs/5 text-fg-muted">
             {questionTypeLabel(question.question_type)}
           </p>

apps/fabro-web/app/components/review-target-question.tsx

@@ -1,76 +1,59 @@
 import { ArrowTopRightOnSquareIcon } from "@heroicons/react/20/solid";
-import {
-  ReviewTargetKind,
-  type ReviewTarget,
-} from "@qltysh/fabro-api-client";
-
-const CONTROL_CHARACTER = /[\u0000-\u001f\u007f-\u009f]/u;
-const UNSAFE_LINK_DELIMITER = /[<>|]/u;
-
-function unicodeScalarCount(value: string): number {
-  return Array.from(value).length;
-}
-
-function hasSafeReviewTarget(target: ReviewTarget | null | undefined): target is ReviewTarget {
-  if (
-    !target ||
-    target.kind !== ReviewTargetKind.DOCUMENT ||
-    !target.label.trim() ||
-    target.label !== target.label.trim() ||
-    unicodeScalarCount(target.label) > 200 ||
-    CONTROL_CHARACTER.test(target.label) ||
-    !target.url ||
-    target.url !== target.url.trim() ||
-    unicodeScalarCount(target.url) > 2048 ||
-    CONTROL_CHARACTER.test(target.url) ||
-    UNSAFE_LINK_DELIMITER.test(target.url)
-  ) {
-    return false;
-  }
+import type { ReviewTarget } from "@qltysh/fabro-api-client";
 
+/**
+ * Re-check the URL before putting it in an `href`. The server already rejects
+ * unsafe targets (see `ReviewTarget::new` in
+ * `lib/foundation/fabro-types/src/interview.rs`), but React does not sanitize
+ * `href`, so a `javascript:` URL reaching this component would execute. Length
+ * and control-character limits stay server-side; they cannot affect the DOM.
+ */
+export function safeReviewTarget(
+  target: ReviewTarget | null | undefined,
+): ReviewTarget | null {
+  if (!target?.url || !target.label) return null;
   try {
     const parsed = new URL(target.url);
-    return (
+    const safe =
       (parsed.protocol === "http:" || parsed.protocol === "https:") &&
       Boolean(parsed.host) &&
       !parsed.username &&
-      !parsed.password
-    );
+      !parsed.password;
+    return safe ? target : null;
   } catch {
-    return false;
+    return null;
   }
 }
 
+/**
+ * The review question sentence, with the target label as an external link.
+ * Mirrors `ReviewTarget::question_text_with_link` in
+ * `lib/foundation/fabro-types/src/interview.rs`.
+ */
 export function ReviewTargetQuestion({
-  reviewTarget,
-  fallbackText,
+  target,
   className,
 }: {
-  reviewTarget: ReviewTarget | null | undefined;
-  fallbackText: string;
+  target: ReviewTarget;
   className?: string;
 }) {
-  if (!hasSafeReviewTarget(reviewTarget)) {
-    return <p className={className}>{fallbackText}</p>;
-  }
-
   return (
     <p className={className}>
       Review the{" "}
       <a
-        href={reviewTarget.url}
+        href={target.url}
         target="_blank"
         rel="noopener noreferrer"
         referrerPolicy="no-referrer"
         className="inline-flex items-baseline gap-1 font-semibold text-teal-300 underline decoration-teal-500/50 underline-offset-2 transition-colors hover:text-fg focus-visible:rounded-sm focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-teal-500"
       >
-        <span>{reviewTarget.label}</span>
+        <span>{target.label}</span>
         <ArrowTopRightOnSquareIcon
           className="size-3 shrink-0 self-center"
           aria-hidden="true"
         />
       </a>{" "}
-      document, then choose the next action.
+      {target.kind}, then choose the next action.
     </p>
   );
 }

apps/fabro-web/app/components/stage-renderers/helpers.ts

@@ -1,10 +1,14 @@
-import { StageOutcome } from "@qltysh/fabro-api-client";
-import type {
-  EventEnvelope,
-  ReviewTarget,
-} from "@qltysh/fabro-api-client";
+import { ReviewTargetKind, StageOutcome } from "@qltysh/fabro-api-client";
+import type { EventEnvelope, ReviewTarget } from "@qltysh/fabro-api-client";
 
-import { getArray, getNumber, getObject, getString, type UnknownRecord } from "../../lib/unknown";
+import {
+  getArray,
+  getNumber,
+  getObject,
+  getString,
+  isRecord,
+  type UnknownRecord,
+} from "../../lib/unknown";
 
 const STAGE_OUTCOMES: ReadonlySet<string> = new Set(Object.values(StageOutcome));
 
@@ -80,12 +84,11 @@ function parseInterviewOptions(value: unknown): InterviewOption[] {
 }
 
 function parseReviewTarget(value: unknown): ReviewTarget | null {
-  if (!value || typeof value !== "object") return null;
-  const record = value as UnknownRecord;
-  const label = getString(record, "label");
-  const url = getString(record, "url");
-  const kind = getString(record, "kind");
-  if (!label || !url || kind !== "document") return null;
+  if (!isRecord(value)) return null;
+  const label = getString(value, "label");
+  const url = getString(value, "url");
+  const kind = getString(value, "kind");
+  if (!label || !url || kind !== ReviewTargetKind.DOCUMENT) return null;
   return { label, url, kind };
 }
 

apps/fabro-web/app/components/stage-renderers/human-qa.tsx

@@ -10,7 +10,10 @@ import {
 import type { EventEnvelope } from "@qltysh/fabro-api-client";
 
 import type { Stage } from "../stage-sidebar";
-import { ReviewTargetQuestion } from "../review-target-question";
+import {
+  ReviewTargetQuestion,
+  safeReviewTarget,
+} from "../review-target-question";
 import { Tooltip } from "../ui";
 import { formatAbsoluteTs, formatDurationMs } from "../../lib/format";
 import { ACTIVE_STAGE_STATES } from "../../lib/stage-sidebar";
@@ -149,6 +152,7 @@ function QuestionBlock({
   stageActive: boolean;
 }) {
   const { question, resolution } = pair;
+  const reviewTarget = safeReviewTarget(question.reviewTarget);
   return (
     <article className="space-y-3">
       <header className="flex flex-wrap items-baseline gap-x-3 gap-y-1">
@@ -169,10 +173,9 @@ function QuestionBlock({
       </header>
 
       <div className="rounded-lg bg-panel p-4 outline-1 -outline-offset-1 outline-line">
-        {question.reviewTarget ? (
+        {reviewTarget ? (
           <ReviewTargetQuestion
-            reviewTarget={question.reviewTarget}
-            fallbackText={question.question}
+            target={reviewTarget}
             className="text-sm/6 text-fg-2"
           />
         ) : (

docs/public/reference/dot-language.mdx

@@ -267,7 +267,7 @@ For the first node in each branch, `fidelity` resolves from the fork-to-branch e
 | Attribute | Type | Description |
 |---|---|---|
 | `question_type` | String | Optional interview question type override: `yes_no`, `confirmation`, `multiple_choice`, `multi_select`, or `freeform`. Defaults to `freeform` when the gate only has a freeform edge; otherwise defaults to `multiple_choice`. |
-| `review_target` | Boolean | When `true`, read and validate the typed `review_target` context value, then present it as the primary link in the question. See [Review targets](/workflows/human-in-the-loop#review-targets). |
+| `review_target` | Boolean | When `true`, read and validate the typed `review_target` context value, then present it as the primary link in the question. Fabro generates the question text, so the node's `label` is not used. See [Review targets](/workflows/human-in-the-loop#review-targets). |
 | `human.default_choice` | String | Target node to use when the question times out. |
 
 ### Manager loop (sub-workflow) nodes

docs/public/workflows/human-in-the-loop.mdx

@@ -99,6 +99,9 @@ Fabro then presents this question:
 The link uses the target URL from context. The example uses a placeholder
 secret, not a live Quarry document.
 
+Fabro generates this question text from the target. A `label` on the gate is
+not used while `review_target=true`.
+
 The target object has three required fields:
 
 | Field | Meaning |

lib/apps/fabro-cli/src/commands/run/attach.rs

@@ -452,13 +452,8 @@ async fn ask_attach_question(question: Question, styles: &'static Styles) -> Ans
         let rendered = styles.render_markdown(context_text);
         eprint!("{rendered}");
     }
-    if let Some(target) = &question.review_target {
-        eprintln!(
-            "Review {}: {} — {}",
-            target.kind().noun(),
-            target.label(),
-            target.url()
-        );
+    if let Some(line) = fabro_interview::review_target_line(&question) {
+        eprintln!("{line}");
     }
     eprintln!("{} {}", styles.bold_cyan.apply_to("?"), question.text);
 

lib/components/fabro-interview/src/console.rs

@@ -111,15 +111,16 @@ fn parse_non_tty_freeform_response(prompt_read: PromptRead) -> Answer {
     }
 }
 
-fn review_target_line(question: &Question) -> Option<String> {
-    question.review_target.as_ref().map(|target| {
-        format!(
-            "Review {}: {} — {}",
-            target.kind().noun(),
-            target.label(),
-            target.url()
-        )
-    })
+/// The review target line printed above a question in terminal clients, which
+/// cannot render a hyperlink label. The label and resource noun are already in
+/// `question.text`, so only the URL is shown. Shared with `fabro-cli`'s attach
+/// client.
+#[must_use]
+pub fn review_target_line(question: &Question) -> Option<String> {
+    question
+        .review_target
+        .as_ref()
+        .map(|target| format!("Review link: {}", target.url()))
 }
 
 /// Ask a multiple-choice question using dialoguer's `Select` widget on a TTY.
@@ -332,26 +333,32 @@ mod tests {
     }
 
     #[test]
-    fn review_target_line_includes_label_and_url() {
-        let mut question = Question::new("Review", QuestionType::MultipleChoice);
-        question.review_target = Some(
-            fabro_types::ReviewTarget::new(
-                "Quarry review exercise",
-                "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
-                fabro_types::ReviewTargetKind::Document,
-            )
-            .unwrap(),
-        );
+    fn review_target_line_shows_only_the_url() {
+        let target = fabro_types::ReviewTarget::new(
+            "Quarry review exercise",
+            "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
+            fabro_types::ReviewTargetKind::Document,
+        )
+        .unwrap();
+        let mut question = Question::new(target.question_text(), QuestionType::MultipleChoice);
+        question.review_target = Some(target);
 
         assert_eq!(
             review_target_line(&question).as_deref(),
             Some(
-                "Review document: Quarry review exercise — \
+                "Review link: \
                  https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef"
             )
         );
     }
 
+    #[test]
+    fn review_target_line_is_absent_without_a_target() {
+        let question = Question::new("Approve?", QuestionType::YesNo);
+
+        assert_eq!(review_target_line(&question), None);
+    }
+
     #[test]
     fn find_matching_option_by_key_case_insensitive() {
         let options = vec![InterviewOption {

lib/components/fabro-interview/src/lib.rs

@@ -222,7 +222,7 @@ pub trait Interviewer: Send + Sync {
 // Re-export all implementors at the crate root
 pub use auto_approve::AutoApproveInterviewer;
 pub use callback::CallbackInterviewer;
-pub use console::ConsoleInterviewer;
+pub use console::{ConsoleInterviewer, review_target_line};
 pub use control::{ControlInterviewer, SubmitError};
 pub use control_protocol::{
     WORKER_CONTROL_INVALID_CURSOR_REASON, WORKER_CONTROL_PONG_TIMEOUT_REASON,

lib/components/fabro-slack/src/blocks.rs

@@ -102,11 +102,8 @@ fn header_section(question: &Question, run_web_url: Option<&str>) -> Value {
     let mut text = question.review_target.as_ref().map_or_else(
         || format!("*{}*", escape_slack_controls(&question.text)),
         |target| {
-            format!(
-                "*Review the {} {}, then choose the next action.*",
-                slack_link(target.url(), target.label()),
-                target.kind().noun()
-            )
+            let link = slack_link(target.url(), target.label());
+            format!("*{}*", target.question_text_with_link(&link))
         },
     );
     if !question.stage.is_empty() {

lib/foundation/fabro-types/src/interview.rs

@@ -7,25 +7,15 @@ use crate::run_event::InterviewOption;
 const REVIEW_TARGET_LABEL_MAX_CHARS: usize = 200;
 const REVIEW_TARGET_URL_MAX_CHARS: usize = 2048;
 
-/// The type of resource a human should review.
-#[derive(
-    Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, strum::Display, strum::EnumString,
-)]
+/// The type of resource a human should review. `Display` renders the noun used
+/// in review question text ("document").
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, strum::Display)]
 #[serde(rename_all = "snake_case")]
 #[strum(serialize_all = "snake_case")]
 pub enum ReviewTargetKind {
     Document,
 }
 
-impl ReviewTargetKind {
-    #[must_use]
-    pub const fn noun(self) -> &'static str {
-        match self {
-            Self::Document => "document",
-        }
-    }
-}
-
 /// A validated external resource presented as the primary subject of a human
 /// review question.
 #[derive(Debug, Clone, PartialEq, Eq, Serialize)]
@@ -71,12 +61,21 @@ impl ReviewTarget {
         self.kind
     }
 
+    /// The question text shown to a human, with the label rendered as plain
+    /// text.
     #[must_use]
     pub fn question_text(&self) -> String {
+        self.question_text_with_link(&self.label)
+    }
+
+    /// The same sentence as [`Self::question_text`], with the label replaced by
+    /// a client-specific rendering of the link (Slack `<url|label>` syntax, for
+    /// example). This is the single definition of the review question wording.
+    #[must_use]
+    pub fn question_text_with_link(&self, rendered_link: &str) -> String {
         format!(
-            "Review the {} {}, then choose the next action.",
-            self.label,
-            self.kind.noun()
+            "Review the {rendered_link} {}, then choose the next action.",
+            self.kind
         )
     }
 }
@@ -93,7 +92,7 @@ pub enum ReviewTargetError {
     EmptyUrl,
     #[error("review target URL must be at most {REVIEW_TARGET_URL_MAX_CHARS} characters")]
     UrlTooLong,
-    #[error("review target URL must not contain control characters or Slack link delimiters")]
+    #[error("review target URL must not contain control characters or link delimiters")]
     UrlContainsUnsafeCharacters,
     #[error("review target URL must be a valid absolute URL")]
     InvalidUrl,
@@ -154,8 +153,10 @@ impl<'de> Deserialize<'de> for ReviewTarget {
     where
         D: serde::Deserializer<'de>,
     {
+        // Unknown fields are ignored so the OpenAPI contract (which leaves
+        // `additionalProperties` permissive) and this deserializer agree, and
+        // so an unknown key in a persisted event cannot fail the whole event.
         #[derive(Deserialize)]
-        #[serde(deny_unknown_fields)]
         struct WireReviewTarget {
             label: String,
             url:   String,

lib/foundation/fabro-types/src/run_event/misc.rs

@@ -1,7 +1,9 @@
 use serde::{Deserialize, Serialize};
 
 use super::ExecOutputTail;
-use crate::{CommandTermination, ParallelBranchResult, PullRequestLink, StageId, StageOutcome};
+use crate::{
+    CommandTermination, ParallelBranchResult, PullRequestLink, ReviewTarget, StageId, StageOutcome,
+};
 
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)]
 pub struct InterviewOption {
@@ -66,7 +68,7 @@ pub struct InterviewStartedProps {
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub context_display: Option<String>,
     #[serde(default, skip_serializing_if = "Option::is_none")]
-    pub review_target:   Option<crate::ReviewTarget>,
+    pub review_target:   Option<ReviewTarget>,
 }
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]